{"thread":{"id":"26911","subject":"[PATCH] Fix potential local deadlock during fetch-pack","startedAt":"2011-03-29T17:06:19Z","lastAt":"2011-03-31T16:24:36Z","messageCount":6,"participants":["Junio C Hamano","Shawn Pearce","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164585","messageId":"7vtyelsvp0.fsf@alter.siamese.dyndns.org","threadId":"26911","inReplyTo":null,"subject":"[PATCH] Fix potential local deadlock during fetch-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-29T17:06:19Z","receivedAt":"2011-03-29T17:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The fetch-pack/upload-pack protocol relies on the underlying transport\n(local pipe or TCP socket) to have enough slack to allow one window worth\nof data in flight without blocking the writer. Traditionally we always\nrelied on being able to have a batch of 32 \"have\"s in flight (roughly 1.5k\nbytes) to stream.\n\nThe recent \"progressive-stride\" change allows \"fetch-pack\" to send up to\n1024 \"have\"s without reading any response from \"upload-pack\". The\noutgoing pipe of \"upload-pack\" can be clogged with many ACK and NAK that\nare unread, while \"fetch-pack\" is still stuffing its outgoing pike with\nmore \"have\"s, leading to a deadlock.\n\nRevert the change unless we are in stateless rpc (aka smart-http) mode, as\nusing a large window full of \"have\"s is still a good way to help reduce\nthe number of back-and-forth, and there is no buffering issue there (it is\nstrictly \"ping-pong\" without an overlap).\n\nSigned-off-by: Junio C Hamano \n---\n builtin/fetch-pack.c | 9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 1724b76..e5fdacd 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -229,16 +229,17 @@ static void insert_alternate_refs(void)\n }\n \n #define INITIAL_FLUSH 16\n+#define PIPESAFE_FLUSH 32\n #define LARGE_FLUSH 1024\n \n static int next_flush(int count)\n {\n-\tif (count < INITIAL_FLUSH * 2)\n-\t\tcount += INITIAL_FLUSH;\n-\telse if (count < LARGE_FLUSH)\n+\tint flush_limit = args.stateless_rpc ? LARGE_FLUSH : PIPESAFE_FLUSH;\n+\n+\tif (count < flush_limit)\n \t\tcount <<= 1;\n \telse\n-\t\tcount += LARGE_FLUSH;\n+\t\tcount += flush_limit;\n \treturn count;\n }\n \n"},{"id":"164587","messageId":"AANLkTikZ=POeQi5nnt8D9v4Z-_OaXNrXDjP2awWUtKzA@mail.gmail.com","threadId":"26911","inReplyTo":"7vtyelsvp0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix potential local deadlock during fetch-pack","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-03-29T17:22:58Z","receivedAt":"2011-03-29T17:22:58Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, Mar 29, 2011 at 10:06, Junio C Hamano wrote:\n> The fetch-pack/upload-pack protocol relies on the underlying transport\n> (local pipe or TCP socket) to have enough slack to allow one window worth\n> of data in flight without blocking the writer.  Traditionally we always\n> relied on being able to have a batch of 32 \"have\"s in flight (roughly 1.5k\n\nIts 64. Because the client \"races ahead\" one window before ever\nreading. Which is closer to 3K of data in flight.\n\n> The recent \"progressive-stride\" change allows \"fetch-pack\" to send up to\n> 1024 \"have\"s without reading any response from \"upload-pack\".  The\n> outgoing pipe of \"upload-pack\" can be clogged with many ACK and NAK that\n> are unread, while \"fetch-pack\" is still stuffing its outgoing pike with\n\ns/pike/pipe/\n\n> @@ -229,16 +229,17 @@ static void insert_alternate_refs(void)\n>  }\n>\n>  #define INITIAL_FLUSH 16\n> +#define PIPESAFE_FLUSH 32\n>  #define LARGE_FLUSH 1024\n>\n>  static int next_flush(int count)\n>  {\n> -       if (count < INITIAL_FLUSH * 2)\n> -               count += INITIAL_FLUSH;\n> -       else if (count < LARGE_FLUSH)\n> +       int flush_limit = args.stateless_rpc ? LARGE_FLUSH : PIPESAFE_FLUSH;\n> +\n> +       if (count < flush_limit)\n>                count <<= 1;\n>        else\n> -               count += LARGE_FLUSH;\n> +               count += flush_limit;\n\nNak. You still deadlock because when count reaches PIPESAFE_FLUSH you\nstill double it to 2*PIPESAFE_FLUSH here. Instead I think you mean:\n\n if (args.stateless_rpc) {\n if (count < LARGE_FLUSH)\n count <<= 1;\n else\n count += LARGE_FLUSH;\n } else {\n if (count * 2 < PIPESAFE_FLUSH)\n count <<= 1;\n }\n\n-- \nShawn.\n"},{"id":"164590","messageId":"AANLkTi=UDPFkox9Gsgv5Bbfsb-VTRweY9D2SNAAhptAc@mail.gmail.com","threadId":"26911","inReplyTo":"AANLkTikZ=POeQi5nnt8D9v4Z-_OaXNrXDjP2awWUtKzA@mail.gmail.com","subject":"Re: [PATCH] Fix potential local deadlock during fetch-pack","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-03-29T17:30:20Z","receivedAt":"2011-03-29T17:30:20Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, Mar 29, 2011 at 10:22, Shawn Pearce wrote:\n> On Tue, Mar 29, 2011 at 10:06, Junio C Hamano wrote:\n>> The fetch-pack/upload-pack protocol relies on the underlying transport\n>> (local pipe or TCP socket) to have enough slack to allow one window worth\n>> of data in flight without blocking the writer.  Traditionally we always\n>> relied on being able to have a batch of 32 \"have\"s in flight (roughly 1.5k\n>\n>> +               count += flush_limit;\n>\n> Nak. You still deadlock because when count reaches PIPESAFE_FLUSH you\n> still double it to 2*PIPESAFE_FLUSH here. Instead I think you mean:\n\nI take this comment back. Re-reading fetch-pack.c the next_flush()\nmethod is accepting as input a running counter of how many have lines\nhave already been sent to the remote peer, and is never reset to 0.\nTherefore it is necessary to add the next round size to count and\nreturn it.\n\n-- \nShawn.\n"},{"id":"164674","messageId":"AANLkTikCRRftjExdhMgFGFMy-Dc1UCMdW1nTYV2bK1-T@mail.gmail.com","threadId":"26911","inReplyTo":"7vtyelsvp0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix potential local deadlock during fetch-pack","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-03-30T09:42:12Z","receivedAt":"2011-03-30T09:42:12Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Mar 29, 2011 at 7:06 PM, Junio C Hamano wrote:\n> The fetch-pack/upload-pack protocol relies on the underlying transport\n> (local pipe or TCP socket) to have enough slack to allow one window worth\n> of data in flight without blocking the writer.  Traditionally we always\n> relied on being able to have a batch of 32 \"have\"s in flight (roughly 1.5k\n> bytes) to stream.\n>\n\nHmm, this explanation makes me wonder: Could this be related to the\ndeadlock we're experiencing with git-push over the git-protocol on\nWindows when side-band-64k is enabled?\n"},{"id":"164676","messageId":"AANLkTinyok+UnR8pOpp43v5EHEN8_YDCC0MFcBfC1LRY@mail.gmail.com","threadId":"26911","inReplyTo":"AANLkTikCRRftjExdhMgFGFMy-Dc1UCMdW1nTYV2bK1-T@mail.gmail.com","subject":"Re: [PATCH] Fix potential local deadlock during fetch-pack","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-03-30T09:50:45Z","receivedAt":"2011-03-30T09:50:45Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Mar 30, 2011 at 11:42 AM, Erik Faye-Lund wrote:\n> On Tue, Mar 29, 2011 at 7:06 PM, Junio C Hamano wrote:\n>> The fetch-pack/upload-pack protocol relies on the underlying transport\n>> (local pipe or TCP socket) to have enough slack to allow one window worth\n>> of data in flight without blocking the writer.  Traditionally we always\n>> relied on being able to have a batch of 32 \"have\"s in flight (roughly 1.5k\n>> bytes) to stream.\n>>\n>\n> Hmm, this explanation makes me wonder: Could this be related to the\n> deadlock we're experiencing with git-push over the git-protocol on\n> Windows when side-band-64k is enabled?\n>\n\nNo, It doesn't seem like that's it. The socket buffers appears to be\n8k by default on Windows, which should be plenty, right?\n\n---8<---\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex d527562..985c271 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1404,7 +1404,7 @@ int mingw_getnameinfo(const struct sockaddr *sa,\nsocklen_t salen,\n\n int mingw_socket(int domain, int type, int protocol)\n {\n-\tint sockfd;\n+\tint sockfd, val, len;\n \tSOCKET s;\n\n \tensure_socket_initialization();\n@@ -1428,6 +1428,12 @@ int mingw_socket(int domain, int type, int protocol)\n \t\treturn error(\"unable to make a socket file descriptor: %s\",\n \t\t\tstrerror(errno));\n \t}\n+\n+\tlen = sizeof(val);\n+\tif (!getsockopt(s, SOL_SOCKET, SO_RCVBUF, (char *)&val, &len))\n+\t\tfprintf(stderr, \"SO_RCVBUF: %d\\n\", val);\n+\tlen = sizeof(val);\n+\tif (!getsockopt(s, SOL_SOCKET, SO_SNDBUF, (char *)&val, &len))\n+\t\tfprintf(stderr, \"SO_SNDBUF: %d\\n\", val);\n \treturn sockfd;\n }\n\n---8<---\n\n$ git push git://localhost\nSO_RCVBUF: 8192\nSO_SNDBUF: 8192\n...\n"},{"id":"164807","messageId":"AANLkTiki2__qAqgDnB6yu8kzxNW-bMj2qJcx50yawCWM@mail.gmail.com","threadId":"26911","inReplyTo":"AANLkTikCRRftjExdhMgFGFMy-Dc1UCMdW1nTYV2bK1-T@mail.gmail.com","subject":"Re: [PATCH] Fix potential local deadlock during fetch-pack","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-03-31T16:24:36Z","receivedAt":"2011-03-31T16:24:36Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Mar 30, 2011 at 02:42, Erik Faye-Lund wrote:\n> On Tue, Mar 29, 2011 at 7:06 PM, Junio C Hamano wrote:\n>> The fetch-pack/upload-pack protocol relies on the underlying transport\n>> (local pipe or TCP socket) to have enough slack to allow one window worth\n>> of data in flight without blocking the writer.  Traditionally we always\n>> relied on being able to have a batch of 32 \"have\"s in flight (roughly 1.5k\n>> bytes) to stream.\n>\n> Hmm, this explanation makes me wonder: Could this be related to the\n> deadlock we're experiencing with git-push over the git-protocol on\n> Windows when side-band-64k is enabled?\n\nI think its unrelated. It might also be a deadlock, but the push\nprotocol is quite a bit different. As far as I remember, there is no\nrisk of deadlock in the push protocol.\n\n-- \nShawn.\n"}]}