{"thread":{"id":"34877","subject":"[PATCH 0/2] upload-pack keepalive","startedAt":"2013-09-08T08:59:15Z","lastAt":"2013-09-09T07:45:29Z","messageCount":4,"participants":["Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"227115","messageId":"20130908085915.GA4097@sigill.intra.peff.net","threadId":"34877","inReplyTo":null,"subject":"[PATCH 0/2] upload-pack keepalive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T08:59:15Z","receivedAt":"2013-09-08T08:59:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I've gotten complaints that cloning from github.com with \"-q\" will\nsometimes abort before sending any data. The problem is that during a\nquiet clone, upload-pack may be silent for a long time while preparing\nthe pack (i.e., the \"counting objects\" and \"compressing\" phases).\n\nWe have load balancers and reverse proxies sitting in front of the git\nmachines, and these machines may sometimes think the connection has hung\nand drop it, even though if they had waited a few more seconds, the pack\nwould have started coming.\n\nWe mitigated it somewhat by bumping the timeouts on our side, but that's\nonly one piece of the puzzle. Clients may be going through http proxies\nor stateful firewalls on their end, and neither we nor they have any\ncontrol.\n\nThis series teaches upload-pack to periodically send an empty sideband\ndata packet when pack-objects is being quiet. That keeps a small amount\nof data flowing, and should be backwards compatible. I hand-tested it\nagainst JGit, dulwich (via the mercurial git plugin), libgit2, and old\nversions of git, and all worked fine.  It has also been running on\ngithub.com for about a week and a half, and nobody has reported any\nproblems.\n\n  [1/2]: upload-pack: send keepalive packets during pack computation\n  [2/2]: upload-pack: bump keepalive default to 5 seconds\n\n-Peff\n"},{"id":"227116","messageId":"20130908090131.GA17060@sigill.intra.peff.net","threadId":"34877","inReplyTo":"20130908085915.GA4097@sigill.intra.peff.net","subject":"[PATCH 1/2] upload-pack: send keepalive packets during pack computation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T09:01:31Z","receivedAt":"2013-09-08T09:01:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Whenn upload-pack has started pack-objects, there may be a\nquiet period while pack-objects prepares the pack (i.e.,\ncounting objects and delta compression).  Normally we would\nsee (and send to the client) progress information, but if\n\"--quiet\" is in effect, pack-objects will produce nothing at\nall until the pack data is ready. On a large repository,\nthis can take tens of seconds (or even minutes if the system\nis loaded or the repository is badly packed). Clients or\nintermediate proxies can sometimes give up in this\nsituation, assuming that the server or connection has hung.\n\nThis patch introduces a \"keepalive\" option; if upload-pack\nsees no data from pack-objects for a certain number of\nseconds, it will send an empty sideband data packet to let\nthe other side know that we are still working on it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt | 11 +++++++++++\n upload-pack.c            | 25 ++++++++++++++++++++++++-\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ec57a15..5d748f7 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2216,6 +2216,17 @@ uploadpack.allowtipsha1inwant::\n \tof a hidden ref (by default, such a request is rejected).\n \tsee also `uploadpack.hiderefs`.\n \n+uploadpack.keepalive::\n+\tWhen `upload-pack` has started `pack-objects`, there may be a\n+\tquiet period while `pack-objects` prepares the pack. Normally\n+\tit would output progress information, but if `--quiet` was used\n+\tfor the fetch, `pack-objects` will output nothing at all until\n+\tthe pack data begins. Some clients and networks may consider\n+\tthe server to be hung and give up. Setting this option instructs\n+\t`upload-pack` to send an empty keepalive packet every\n+\t`uploadpack.keepalive` seconds. Setting this option to 0\n+\tdisables keepalive packets entirely. The default is 0.\n+\n url.<base>.insteadOf::\n \tAny URL that starts with this value will be rewritten to\n \tstart, instead, with <base>. In cases where some site serves a\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 127e59a..c3e4a20 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -40,6 +40,7 @@ static unsigned int timeout;\n static struct object_array want_obj;\n static struct object_array extra_edge_obj;\n static unsigned int timeout;\n+static int keepalive = -1;\n /* 0 for no sideband,\n  * otherwise maximum packet size (up to 65520 bytes).\n  */\n@@ -200,6 +201,7 @@ static void create_pack_file(void)\n \twhile (1) {\n \t\tstruct pollfd pfd[2];\n \t\tint pe, pu, pollsize;\n+\t\tint ret;\n \n \t\treset_timeout();\n \n@@ -222,7 +224,8 @@ static void create_pack_file(void)\n \t\tif (!pollsize)\n \t\t\tbreak;\n \n-\t\tif (poll(pfd, pollsize, -1) < 0) {\n+\t\tret = poll(pfd, pollsize, 1000 * keepalive);\n+\t\tif (ret < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\terror(\"poll failed, resuming: %s\",\n \t\t\t\t      strerror(errno));\n@@ -284,6 +287,21 @@ static void create_pack_file(void)\n \t\t\tif (sz < 0)\n \t\t\t\tgoto fail;\n \t\t}\n+\n+\t\t/*\n+\t\t * We hit the keepalive timeout without saying anything; send\n+\t\t * an empty message on the data sideband just to let the other\n+\t\t * side know we're still working on it, but don't have any data\n+\t\t * yet.\n+\t\t *\n+\t\t * If we don't have a sideband channel, there's no room in the\n+\t\t * protocol to say anything, so those clients are just out of\n+\t\t * luck.\n+\t\t */\n+\t\tif (!ret && use_sideband) {\n+\t\t\tstatic const char buf[] = \"0005\\1\";\n+\t\t\twrite_or_die(1, buf, 5);\n+\t\t}\n \t}\n \n \tif (finish_command(&pack_objects)) {\n@@ -785,6 +803,11 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n {\n \tif (!strcmp(\"uploadpack.allowtipsha1inwant\", var))\n \t\tallow_tip_sha1_in_want = git_config_bool(var, value);\n+\telse if (!strcmp(\"uploadpack.keepalive\", var)) {\n+\t\tkeepalive = git_config_int(var, value);\n+\t\tif (!keepalive)\n+\t\t\tkeepalive = -1;\n+\t}\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-- \n1.8.4.2.g87d4a77\n"},{"id":"227117","messageId":"20130908090206.GB17060@sigill.intra.peff.net","threadId":"34877","inReplyTo":"20130908085915.GA4097@sigill.intra.peff.net","subject":"[PATCH 2/2] upload-pack: bump keepalive default to 5 seconds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T09:02:06Z","receivedAt":"2013-09-08T09:02:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nThere is no reason not to turn on keepalives by default.\nThey take very little bandwidth, and significantly less than\nthe progress reporting they are replacing. And in the case\nthat progress reporting is on, we should never need to send\na keepalive anyway, as we will constantly be showing\nprogress and resetting the keepalive timer.\n\nWe do not necessarily know what the client's idea of a\nreasonable timeout is, so let's keep this on the low side of\n5 seconds. That is high enough that we will always prefer\nour normal 1-second progress reports to sending a keepalive\npacket, but low enough that no sane client should consider\nthe connection hung.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt | 2 +-\n upload-pack.c            | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 5d748f7..6b35578 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2225,7 +2225,7 @@ uploadpack.keepalive::\n \tthe server to be hung and give up. Setting this option instructs\n \t`upload-pack` to send an empty keepalive packet every\n \t`uploadpack.keepalive` seconds. Setting this option to 0\n-\tdisables keepalive packets entirely. The default is 0.\n+\tdisables keepalive packets entirely. The default is 5 seconds.\n \n url.<base>.insteadOf::\n \tAny URL that starts with this value will be rewritten to\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c3e4a20..04a8707 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -40,7 +40,7 @@ static unsigned int timeout;\n static struct object_array want_obj;\n static struct object_array extra_edge_obj;\n static unsigned int timeout;\n-static int keepalive = -1;\n+static int keepalive = 5;\n /* 0 for no sideband,\n  * otherwise maximum packet size (up to 65520 bytes).\n  */\n-- \n1.8.4.2.g87d4a77\n"},{"id":"227204","messageId":"CAPig+cTa2QYtQZ-kFCTTvhHC+saJS1o=5CyFwfwARXxg8sn-uA@mail.gmail.com","threadId":"34877","inReplyTo":"20130908090131.GA17060@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] upload-pack: send keepalive packets during pack computation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-09T07:45:29Z","receivedAt":"2013-09-09T07:45:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 8, 2013 at 5:01 AM, Jeff King <peff@peff.net> wrote:\n> Whenn upload-pack has started pack-objects, there may be a\n\ns/Whenn/When/\n\n> quiet period while pack-objects prepares the pack (i.e.,\n> counting objects and delta compression).  Normally we would\n> see (and send to the client) progress information, but if\n> \"--quiet\" is in effect, pack-objects will produce nothing at\n> all until the pack data is ready. On a large repository,\n> this can take tens of seconds (or even minutes if the system\n> is loaded or the repository is badly packed). Clients or\n> intermediate proxies can sometimes give up in this\n> situation, assuming that the server or connection has hung.\n>\n> This patch introduces a \"keepalive\" option; if upload-pack\n> sees no data from pack-objects for a certain number of\n> seconds, it will send an empty sideband data packet to let\n> the other side know that we are still working on it.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"}]}