{"thread":{"id":"35132","subject":"[PATCH] http: enable keepalive on TCP sockets","startedAt":"2013-10-12T22:29:40Z","lastAt":"2013-10-15T06:03:22Z","messageCount":9,"participants":["Eric Wong","Daniel Stenberg","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228889","messageId":"20131012222939.GA24255@dcvr.yhbt.net","threadId":"35132","inReplyTo":null,"subject":"[PATCH] http: enable keepalive on TCP sockets","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2013-10-12T22:29:40Z","receivedAt":"2013-10-12T22:29:40Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This is a follow up to commit e47a8583a20256851e7fc882233e3bd5bf33dc6e\n(enable SO_KEEPALIVE for connected TCP sockets).\n\nSockets may never receive notification of some link errors,\ncausing \"git fetch\" or similar processes to hang forever.\nEnabling keepalive messages allows hung processes to error out\nafter a few minutes/hours depending on the keepalive settings of\nthe system.\n\nI noticed this problem with some non-interactive cronjobs getting\nhung when talking to HTTP servers.\n\nSigned-off-by: Eric Wong \n---\n http.c | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex f3e1439..5834c9b 100644\n--- a/http.c\n+++ b/http.c\n@@ -260,6 +260,24 @@ static int has_cert_password(void)\n \treturn 1;\n }\n \n+/* curl 7.25.0 has CURLOPT_TCP_KEEPALIVE, too, but we support older curl */\n+static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)\n+{\n+\tint ka = 1;\n+\tint rc;\n+\tsocklen_t len = (socklen_t)sizeof(ka);\n+\n+\tif (type != CURLSOCKTYPE_IPCXN)\n+\t\treturn 0;\n+\n+\trc = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&ka, len);\n+\tif (rc < 0)\n+\t\twarning(\"unable to set SO_KEEPALIVE on socket %s\",\n+\t\t\tstrerror(errno));\n+\n+\treturn 0; /* CURL_SOCKOPT_OK only exists since curl 7.21.5 */\n+}\n+\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n@@ -332,6 +350,10 @@ static CURL *get_curl_handle(void)\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n \t}\n \n+#if LIBCURL_VERSION_NUM >= 0x071000\n+\tcurl_easy_setopt(result, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);\n+#endif\n+\n \treturn result;\n }\n \n-- \nEric Wong\n"},{"id":"228896","messageId":"alpine.DEB.2.00.1310131142080.22193@tvnag.unkk.fr","threadId":"35132","inReplyTo":"20131012222939.GA24255@dcvr.yhbt.net","subject":"Re: [PATCH] http: enable keepalive on TCP sockets","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2013-10-13T09:44:09Z","receivedAt":"2013-10-13T09:44:09Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Sat, 12 Oct 2013, Eric Wong wrote:\n\n> This is a follow up to commit e47a8583a20256851e7fc882233e3bd5bf33dc6e \n> (enable SO_KEEPALIVE for connected TCP sockets).\n\nJust keep in mind that TCP keep-alive is enabled in awkwardly many different \nways on different systems and this patch only supports one of them. Feel free \nto take inspiration from libcurl's source code for doing this. See:\n\n https://github.com/bagder/curl/blob/master/lib/connect.c#L108\n\n-- \n\n / daniel.haxx.se\n"},{"id":"228921","messageId":"20131014052739.GA16129@dcvr.yhbt.net","threadId":"35132","inReplyTo":"alpine.DEB.2.00.1310131142080.22193@tvnag.unkk.fr","subject":"Re: [PATCH] http: enable keepalive on TCP sockets","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2013-10-14T05:27:39Z","receivedAt":"2013-10-14T05:27:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Daniel Stenberg wrote:\n> On Sat, 12 Oct 2013, Eric Wong wrote:\n> \n> >This is a follow up to commit\n> >e47a8583a20256851e7fc882233e3bd5bf33dc6e (enable SO_KEEPALIVE for\n> >connected TCP sockets).\n> \n> Just keep in mind that TCP keep-alive is enabled in awkwardly many\n> different ways on different systems and this patch only supports one\n> of them. Feel free to take inspiration from libcurl's source code\n> for doing this. See:\n> \n> https://github.com/bagder/curl/blob/master/lib/connect.c#L108\n\nThanks. I think the Linux-specific TCP_KEEP* knobs are overkill for git.\n(since this is mainly for non-interactive users, I went at least a day\n before realizing the process was stuck on my machine).\nI cannot comment on the knobs for other OSes.\n"},{"id":"228948","messageId":"20131014214035.GB7007@sigill.intra.peff.net","threadId":"35132","inReplyTo":"20131014052739.GA16129@dcvr.yhbt.net","subject":"Re: [PATCH] http: enable keepalive on TCP sockets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-14T21:40:35Z","receivedAt":"2013-10-14T21:40:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 14, 2013 at 05:27:39AM +0000, Eric Wong wrote:\n\n> Daniel Stenberg wrote:\n> > On Sat, 12 Oct 2013, Eric Wong wrote:\n> > \n> > >This is a follow up to commit\n> > >e47a8583a20256851e7fc882233e3bd5bf33dc6e (enable SO_KEEPALIVE for\n> > >connected TCP sockets).\n> > \n> > Just keep in mind that TCP keep-alive is enabled in awkwardly many\n> > different ways on different systems and this patch only supports one\n> > of them. Feel free to take inspiration from libcurl's source code\n> > for doing this. See:\n> > \n> > https://github.com/bagder/curl/blob/master/lib/connect.c#L108\n> \n> Thanks. I think the Linux-specific TCP_KEEP* knobs are overkill for git.\n> (since this is mainly for non-interactive users, I went at least a day\n> before realizing the process was stuck on my machine).\n> I cannot comment on the knobs for other OSes.\n\nI don't think we should get into having a big compatibility layer that\njust reproduces what is in curl.\n\nBut is there any reason not to use CURLOPT_TCP_KEEPALIVE when it is\navailable, falling back to CURLOPT_SOCKOPTFUNCTION, and then finally to\nnothing? That lets people on modern curl benefit from curl's more\nportable code, without punishing people on older versions.\n\nI.e., something like:\n\n---\ndiff --git a/http.c b/http.c\nindex 5834c9b..e221efb 100644\n--- a/http.c\n+++ b/http.c\n@@ -254,36 +254,54 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)\n \tif (!cert_auth.password) {\n \t\tcert_auth.protocol = xstrdup(\"cert\");\n \t\tcert_auth.username = xstrdup(\"\");\n \t\tcert_auth.path = xstrdup(ssl_cert);\n \t\tcredential_fill(&cert_auth);\n \t}\n \treturn 1;\n }\n \n-/* curl 7.25.0 has CURLOPT_TCP_KEEPALIVE, too, but we support older curl */\n+#if LIBCURL_VERSION_NUM >= 0x071900\n+static void set_curl_keepalive(CURL *c)\n+{\n+\tcurl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);\n+}\n+\n+#elif LIBCURL_VERSION_NUM >= 0x071000\n static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)\n {\n \tint ka = 1;\n \tint rc;\n \tsocklen_t len = (socklen_t)sizeof(ka);\n \n \tif (type != CURLSOCKTYPE_IPCXN)\n \t\treturn 0;\n \n \trc = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&ka, len);\n \tif (rc < 0)\n \t\twarning(\"unable to set SO_KEEPALIVE on socket %s\",\n \t\t\tstrerror(errno));\n \n \treturn 0; /* CURL_SOCKOPT_OK only exists since curl 7.21.5 */\n }\n \n+static void set_curl_keepalive(CURL *c)\n+{\n+\tcurl_easy_setopt(c, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);\n+}\n+\n+#else\n+static void set_curl_keepalive(CURL *c)\n+{\n+\t/* not supported on older curl versions */\n+}\n+#endif\n+\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n \n \tif (!curl_ssl_verify) {\n \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);\n \t\tcurl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);\n \t} else {\n \t\t/* Verify authenticity of the peer's certificate */\n@@ -344,21 +362,19 @@ static CURL *get_curl_handle(void)\n \tif (curl_ssl_try)\n \t\tcurl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);\n #endif\n \n \tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n \t}\n \n-#if LIBCURL_VERSION_NUM >= 0x071000\n-\tcurl_easy_setopt(result, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);\n-#endif\n+\tset_curl_keepalive(result);\n \n \treturn result;\n }\n \n static void set_from_env(const char **var, const char *envname)\n {\n \tconst char *val = getenv(envname);\n \tif (val)\n \t\t*var = val;\n"},{"id":"228978","messageId":"20131014233839.GA26323@dcvr.yhbt.net","threadId":"35132","inReplyTo":"20131014214035.GB7007@sigill.intra.peff.net","subject":"Re: [PATCH] http: enable keepalive on TCP sockets","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2013-10-14T23:38:39Z","receivedAt":"2013-10-14T23:38:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Mon, Oct 14, 2013 at 05:27:39AM +0000, Eric Wong wrote:\n> > Daniel Stenberg wrote:\n> > > On Sat, 12 Oct 2013, Eric Wong wrote:\n> > > \n> > > >This is a follow up to commit\n> > > >e47a8583a20256851e7fc882233e3bd5bf33dc6e (enable SO_KEEPALIVE for\n> > > >connected TCP sockets).\n> > > \n> > > Just keep in mind that TCP keep-alive is enabled in awkwardly many\n> > > different ways on different systems and this patch only supports one\n> > > of them. Feel free to take inspiration from libcurl's source code\n> > > for doing this. See:\n> > > \n> > > https://github.com/bagder/curl/blob/master/lib/connect.c#L108\n> > \n> > Thanks. I think the Linux-specific TCP_KEEP* knobs are overkill for git.\n> > (since this is mainly for non-interactive users, I went at least a day\n> > before realizing the process was stuck on my machine).\n> > I cannot comment on the knobs for other OSes.\n> \n> I don't think we should get into having a big compatibility layer that\n> just reproduces what is in curl.\n> \n> But is there any reason not to use CURLOPT_TCP_KEEPALIVE when it is\n> available, falling back to CURLOPT_SOCKOPTFUNCTION, and then finally to\n> nothing? That lets people on modern curl benefit from curl's more\n> portable code, without punishing people on older versions.\n\nI wanted it to work as older curl first (since I noticed this\non an old server). But your patch on top of mine looks reasonable,\nthanks.\n"},{"id":"228981","messageId":"20131015000614.GA10905@sigill.intra.peff.net","threadId":"35132","inReplyTo":"20131014233839.GA26323@dcvr.yhbt.net","subject":"[PATCH] http: use curl's tcp keepalive if available","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-15T00:06:14Z","receivedAt":"2013-10-15T00:06:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 14, 2013 at 11:38:39PM +0000, Eric Wong wrote:\n\n> I wanted it to work as older curl first (since I noticed this\n> on an old server). But your patch on top of mine looks reasonable,\n> thanks.\n\nMakes sense. Here it is with a real commit message (on top of the\new/keepalive topic).\n\n-- >8 --\nSubject: http: use curl's tcp keepalive if available\n\nCommit a15d069 taught git to use curl's SOCKOPTFUNCTION hook\nto turn on TCP keepalives. However, modern versions of curl\nhave a TCP_KEEPALIVE option, which can do this for us. As an\nadded bonus, the curl code knows how to turn on keepalive\nfor a much wider variety of platforms. The only downside to\nusing this option is that not everybody has a new enough curl.\nLet's split our keepalive options into three conditionals:\n\n 1. With curl 7.25.0 and newer, we rely on curl to do it\n right.\n\n 2. With older curl that still knows SOCKOPTFUNCTION, we\n use the code from a15d069.\n\n 3. Otherwise, we are out of luck, and the call is a no-op.\n\nSigned-off-by: Jeff King \n---\nGiven the #ifdefs in curl's keepalive code, I suspect we may see build\nproblems for people in case 2 on some systems, with or without my patch.\nI think this patch is a strict improvement, though; if they have a new\nenough curl, they will not even look at the case 2 code. And if they do\nnot, our previous options were:\n\n a. Add platform-specific code for them.\n \n b. Tell them they are out of luck, and add an #ifdef to push them into\n case 3.\n\nNow we have an extra option:\n\n c. Tell them to upgrade curl. :)\n\n http.c | 24 ++++++++++++++++++++----\n 1 file changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a2c1819..6359526 100644\n--- a/http.c\n+++ b/http.c\n@@ -233,7 +233,13 @@ static int has_cert_password(void)\n \t\treturn 0;\n }\n \n-/* curl 7.25.0 has CURLOPT_TCP_KEEPALIVE, too, but we support older curl */\n+#if LIBCURL_VERSION_NUM >= 0x071900\n+static void set_curl_keepalive(CURL *c)\n+{\n+\tcurl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);\n+}\n+\n+#elif LIBCURL_VERSION_NUM >= 0x071000\n static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)\n {\n \tint ka = 1;\n@@ -251,6 +257,18 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)\n \treturn 0; /* CURL_SOCKOPT_OK only exists since curl 7.21.5 */\n }\n \n+static void set_curl_keepalive(CURL *c)\n+{\n+\tcurl_easy_setopt(c, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);\n+}\n+\n+#else\n+static void set_curl_keepalive(CURL *c)\n+{\n+\t/* not supported on older curl versions */\n+}\n+#endif\n+\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n@@ -316,9 +334,7 @@ static CURL *get_curl_handle(void)\n \tif (curl_http_proxy)\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \n-#if LIBCURL_VERSION_NUM >= 0x071000\n-\tcurl_easy_setopt(result, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);\n-#endif\n+\tset_curl_keepalive(result);\n \n \treturn result;\n }\n-- \n1.8.4.1.4.gf327177\n"},{"id":"228990","messageId":"20131015045814.GA12312@dcvr.yhbt.net","threadId":"35132","inReplyTo":"20131015000614.GA10905@sigill.intra.peff.net","subject":"Re: [PATCH] http: use curl's tcp keepalive if available","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2013-10-15T04:58:14Z","receivedAt":"2013-10-15T04:58:14Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Mon, Oct 14, 2013 at 11:38:39PM +0000, Eric Wong wrote:\n> \n> > I wanted it to work as older curl first (since I noticed this\n> > on an old server). But your patch on top of mine looks reasonable,\n> > thanks.\n> \n> Makes sense. Here it is with a real commit message (on top of the\n> ew/keepalive topic).\n> \n> -- >8 --\n> Subject: http: use curl's tcp keepalive if available\n> \n> Commit a15d069 taught git to use curl's SOCKOPTFUNCTION hook\n> to turn on TCP keepalives. However, modern versions of curl\n> have a TCP_KEEPALIVE option, which can do this for us. As an\n> added bonus, the curl code knows how to turn on keepalive\n> for a much wider variety of platforms. The only downside to\n> using this option is that not everybody has a new enough curl.\n> Let's split our keepalive options into three conditionals:\n> \n> 1. With curl 7.25.0 and newer, we rely on curl to do it\n> right.\n> \n> 2. With older curl that still knows SOCKOPTFUNCTION, we\n> use the code from a15d069.\n> \n> 3. Otherwise, we are out of luck, and the call is a no-op.\n> \n> Signed-off-by: Jeff King \n\nThanks.\nTested-by: Eric Wong \non curl 7.21.0 and 7.26.0, confirmed via strace:\n\n\tsetsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, [1], 4) = 0\n\nI can also confirm 7.26.0 adds:\n\n\tsetsockopt(fd, SOL_TCP, TCP_KEEPIDLE, [60], 4) = 0\n\tsetsockopt(fd, SOL_TCP, TCP_KEEPINTVL, [60], 4) = 0\n\nto the strace on Linux.\n\n> ---\n> Given the #ifdefs in curl's keepalive code, I suspect we may see build\n> problems for people in case 2 on some systems, with or without my patch.\n> I think this patch is a strict improvement, though; if they have a new\n> enough curl, they will not even look at the case 2 code. And if they do\n> not, our previous options were:\n> \n> a. Add platform-specific code for them.\n> \n> b. Tell them they are out of luck, and add an #ifdef to push them into\n> case 3.\n\nCase 2 works fine on my Debian Squeeze system.\n\n> Now we have an extra option:\n> \n> c. Tell them to upgrade curl. :)\n\nYes, I keep forgetting I still have a Debian Squeeze machine :x\n"},{"id":"228991","messageId":"20131015050028.GA8150@sigill.intra.peff.net","threadId":"35132","inReplyTo":"20131015045814.GA12312@dcvr.yhbt.net","subject":"Re: [PATCH] http: use curl's tcp keepalive if available","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-15T05:00:28Z","receivedAt":"2013-10-15T05:00:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 15, 2013 at 04:58:14AM +0000, Eric Wong wrote:\n\n> > Subject: http: use curl's tcp keepalive if available\n> [...]\n> Tested-by: Eric Wong \n> on curl 7.21.0 and 7.26.0, confirmed via strace:\n> \n> \tsetsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, [1], 4) = 0\n> \n> I can also confirm 7.26.0 adds:\n> \n> \tsetsockopt(fd, SOL_TCP, TCP_KEEPIDLE, [60], 4) = 0\n> \tsetsockopt(fd, SOL_TCP, TCP_KEEPINTVL, [60], 4) = 0\n> \n> to the strace on Linux.\n\nThanks, I didn't actually do any testing myself.\n\n> Case 2 works fine on my Debian Squeeze system.\n> \n> > Now we have an extra option:\n> > \n> > c. Tell them to upgrade curl. :)\n> \n> Yes, I keep forgetting I still have a Debian Squeeze machine :x\n\nI am more concerned with Windows, which may not compile your original\npatch at all.\n\n-Peff\n"},{"id":"228994","messageId":"20131015060322.GA10714@dcvr.yhbt.net","threadId":"35132","inReplyTo":"20131015050028.GA8150@sigill.intra.peff.net","subject":"Re: [PATCH] http: use curl's tcp keepalive if available","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2013-10-15T06:03:22Z","receivedAt":"2013-10-15T06:03:22Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> I am more concerned with Windows, which may not compile your original\n> patch at all.\n\nThe code I introduced in e47a8583a20256851e7fc882233e3bd5bf33dc6e\n(\"enable SO_KEEPALIVE for connected TCP sockets\" Dec 2011)\ndidn't trigger the addition of any new #ifdef guards. I think we're\nfine (but I'll let others confirm it).\n"}]}