{"thread":{"id":"26983","subject":"[RFC] upload-pack deadlock","startedAt":"2011-04-04T05:36:26Z","lastAt":"2011-05-26T17:11:10Z","messageCount":10,"participants":["Jeff King","Junio C Hamano","Erik Faye-Lund","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"165089","messageId":"20110404053626.GA26529@sigill.intra.peff.net","threadId":"26983","inReplyTo":null,"subject":"[RFC] upload-pack deadlock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-04T05:36:26Z","receivedAt":"2011-04-04T05:36:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In a pthread-enabled version of upload-pack, there's a race condition\nthat can cause a deadlock on the fflush(NULL) we call from run-command.\n\nWhat happens is this:\n\n 1. Upload-pack is informed we are doing a shallow clone.\n\n 2. We call start_async() to spawn a thread that will generate rev-list\n results to feed to pack-objects. It gets a file descriptor to a\n pipe which will eventually hook to pack-objects.\n\n 3. The rev-list thread uses fdopen to create a new output stream\n around the fd we gave it, called pack_pipe.\n\n 4. The thread writes results to pack_pipe. Outside of our control,\n libc is doing locking on the stream. We keep writing until the OS\n pipe buffer is full, and then we block in write(), still holding\n the lock.\n\n 5. The main thread now uses start_command to spawn pack-objects.\n Before forking, it calls fflush(NULL) to flush every stdio output\n buffer. It blocks trying to get the lock on pack_pipe.\n\nAnd we have a deadlock. The thread will block until somebody starts\nreading from the pipe. But nobody will read from the pipe until we\nfinish flushing to the pipe.\n\nMost of the time it doesn't happen, because the main thread manages to\ncall fflush(NULL) before the rev-list thread manages to block. But it's\neasy to reproduce if you apply this patch:\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex bba053f..ceaef86 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -189,6 +189,7 @@ static void create_pack_file(void)\n \tpack_objects.git_cmd = 1;\n \tpack_objects.argv = argv;\n \n+\tsleep(1);\n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n \n\nwhich slows the main thread just enough. With that applied, I can\nreproduce reliably by running:\n\n git clone --depth 1 file://$GIT\n\nwhere $GIT is some repo that is large enough that its rev-list output\nwill fill the OS's pipe buffer (I used git.git).\n\nThe end of the strace looks like this (27315 is the parent, 27316 is the\nthread):\n\n[pid 27316] write(4, \"0ac7ec43918a5af59f1a305d47cd16ed\"..., 4096) = 4096\n[pid 27316] write(4, \"entation/RelNotes/1.5.3.txt\\nd4e4\"..., 4096) = 4096\n[pid 27316] write(4, \"elNotes/1.6.5.3.txt\\nd3a2a3e71243\"..., 4096) = 4096\n[pid 27316] write(4, \"cumentation/docbook.xsl\\nae413e52\"..., 4096) = 4096\n[pid 27316] write(4, \"5d5c06cc37c7a203f758 Documentati\"..., 4096 \n[pid 27315] <... nanosleep resumed> 0x7fff053d81e0) = 0\n[pid 27315] pipe([5, 6]) = 0\n[pid 27315] pipe([7, 8]) = 0\n[pid 27315] futex(0x82c930, FUTEX_WAIT_PRIVATE, 2, NULL\n\nSo you see the thread blocking on write, and the parent process stuck\ntrying to get the lock.\n\nThere are a few possible solutions:\n\n 1. Flip the order of initialization, so that we don't start writing\n anything until the pack-objects reader is already in place. That\n works in this case, and the patch is at the end of this mail. The\n biggest problem is that it doesn't fix the general case.\n\n 2. Rewrite the async rev-list into a separate rev-list invocation.\n There is no deadlock without threads (and this works fine if\n start_async uses fork() on your system). The problem is that I'm\n not sure what the thread is doing is possible via a rev-list\n invocation. And it's a lot more error-prone than (1), but still\n doesn't fix the general case.\n\n 3. Make run-command fflush just stdout and stderr before calling fork.\n The reasons to flush before forking are:\n\n a. Avoid duplicated output when the forked process might be\n writing to the stream (and then both sides will end up\n outputting whatever cruft was in the buffer). But the point of\n run-command is to exec something, so the only stream we are\n likely to write to before exec'ing something (and losing those\n buffers anyway) is going to be stderr.\n\n b. Provide sane ordering of output. If you write something to\n a stream and then run a new command, you expect your output to\n come before the sub-command's.\n\n Doing (a) is easy; we really just need to flush stderr. Doing (b)\n is harder. Most of the time the buffer you would want to flush is\n going to be stdout. But it's possible for it to be some other FILE\n stream pointing to a pipe.\n\n So the best we can do is to flush the common things that are\n probably safe, and then any callers who really care about the\n ordering of other output will have to fflush() their outputs\n manually.\n\n It would be nice to do the opposite: have everything flushed unless\n the caller says \"no, this stream is being taken care of in a\n thread, so don't worry about it\". But I don't think there is any\n way to do that. fflush(NULL) flushes everything, and there is no\n way to even get a list of what it _would_ flush so that we could do\n it ourselves.\n\n But of course that doesn't really fix the problem. It just\n restricts it to stdout and stderr. But once we have the actual list\n of things to flush, we can use ftrylockfile() to just try flushing\n each one (ftrylockfile is probably not available everywhere, but it\n is in POSIX.1-2001, which hopefully means it's available where we\n have pthreads).\n\nI am tempted to go with (1), as it's simple and unlikely to introduce\nnew bugs. There are a few other start_async callsites with threads that\noutput to stdio: demuxers which call recv_sideband. They write to\nstderr, but since they are just relaying stderr from the remote side, it\nis unlikely that a well-behaved remote would send enough to actually\ncause stderr to block. OTOH, I know Erik has mentioned some deadlock\nproblems on Windows when sideband-64k is enabled, so that might be\nrelated (we start_async the demuxer and then spawn pack-objects).\n\nComments/ideas? Patch for approach (1) is below.\n\n---\ndiff --git a/upload-pack.c b/upload-pack.c\nindex bba053f..ce5cbbe 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -157,15 +157,8 @@ static void create_pack_file(void)\n \tconst char *argv[10];\n \tint arg = 0;\n \n-\tif (shallow_nr) {\n-\t\tmemset(&rev_list, 0, sizeof(rev_list));\n-\t\trev_list.proc = do_rev_list;\n-\t\trev_list.out = -1;\n-\t\tif (start_async(&rev_list))\n-\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n-\t\targv[arg++] = \"pack-objects\";\n-\t} else {\n-\t\targv[arg++] = \"pack-objects\";\n+\targv[arg++] = \"pack-objects\";\n+\tif (!shallow_nr) {\n \t\targv[arg++] = \"--revs\";\n \t\tif (create_full_pack)\n \t\t\targv[arg++] = \"--all\";\n@@ -183,7 +176,7 @@ static void create_pack_file(void)\n \targv[arg++] = NULL;\n \n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n-\tpack_objects.in = shallow_nr ? rev_list.out : -1;\n+\tpack_objects.in = -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n@@ -192,8 +185,14 @@ static void create_pack_file(void)\n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n \n-\t/* pass on revisions we (don't) want */\n-\tif (!shallow_nr) {\n+\tif (shallow_nr) {\n+\t\tmemset(&rev_list, 0, sizeof(rev_list));\n+\t\trev_list.proc = do_rev_list;\n+\t\trev_list.out = pack_objects.in;\n+\t\tif (start_async(&rev_list))\n+\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n+\t}\n+\telse {\n \t\tFILE *pipe_fd = xfdopen(pack_objects.in, \"w\");\n \t\tif (!create_full_pack) {\n \t\t\tint i;\n"},{"id":"165290","messageId":"7v8vvnjnyg.fsf@alter.siamese.dyndns.org","threadId":"26983","inReplyTo":"20110404053626.GA26529@sigill.intra.peff.net","subject":"Re: [RFC] upload-pack deadlock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-06T17:20:55Z","receivedAt":"2011-04-06T17:20:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In a pthread-enabled version of upload-pack, there's a race condition\n> that can cause a deadlock on the fflush(NULL) we call from run-command.\n>\n> What happens is this:\n>\n> 1. Upload-pack is informed we are doing a shallow clone.\n>\n> 2. We call start_async() to spawn a thread that will generate rev-list\n> results to feed to pack-objects. It gets a file descriptor to a\n> pipe which will eventually hook to pack-objects.\n>\n> 3. The rev-list thread uses fdopen to create a new output stream\n> around the fd we gave it, called pack_pipe.\n>\n> 4. The thread writes results to pack_pipe. Outside of our control,\n> libc is doing locking on the stream. We keep writing until the OS\n> pipe buffer is full, and then we block in write(), still holding\n> the lock.\n>\n> 5. The main thread now uses start_command to spawn pack-objects.\n> Before forking, it calls fflush(NULL) to flush every stdio output\n> buffer. It blocks trying to get the lock on pack_pipe.\n>\n> And we have a deadlock. The thread will block until somebody starts\n> reading from the pipe. But nobody will read from the pipe until we\n> finish flushing to the pipe.\n\nThanks for a concise summary.\n\n> There are a few possible solutions:\n>\n> 1. Flip the order of initialization, so that we don't start writing\n> anything until the pack-objects reader is already in place. That\n> works in this case, and the patch is at the end of this mail. The\n> biggest problem is that it doesn't fix the general case.\n\nIn what sense are you not fixing \"the general case\", though?\n\nIf a program runs two threads, both of which access the FILE streams, it\nshould be the responsibility of the program to get these threads\ncoordinated to avoid such races and deadlocks, no?\n\nWhile I think you mean start_async() API should be helping the API users\n(calling programs) to fulfil that responsibility better, it really is up\nto the thread to do random things to wreak havoc, like the shallow\ncodepath that tries to fill the stream while the main codepath expected\nnothing happening, and I do not think of a good abstraction to help us\nbeing more careful.\n"},{"id":"165292","messageId":"20110406175413.GA8205@sigill.intra.peff.net","threadId":"26983","inReplyTo":"7v8vvnjnyg.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] upload-pack deadlock","fromName":"Jeff King","fromEmail":"peff@github.com","sentAt":"2011-04-06T17:54:13Z","receivedAt":"2011-04-06T17:54:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 06, 2011 at 10:20:55AM -0700, Junio C Hamano wrote:\n\n> > There are a few possible solutions:\n> >\n> > 1. Flip the order of initialization, so that we don't start writing\n> > anything until the pack-objects reader is already in place. That\n> > works in this case, and the patch is at the end of this mail. The\n> > biggest problem is that it doesn't fix the general case.\n> \n> In what sense are you not fixing \"the general case\", though?\n> \n> If a program runs two threads, both of which access the FILE streams, it\n> should be the responsibility of the program to get these threads\n> coordinated to avoid such races and deadlocks, no?\n\nYes, but the problem is that looking at the code of the two threads, you\nwould never realize there is a deadlock. They never intentionally try to\ntouch the same stream. The real problem is buried in the run-command\ncode which calls fflush(NULL). This is inherently thread-unsafe, because\nit wants to touch global data. It does do proper locking, at least, but\nthere is a deadlock issue, as demonstrated here.\n\nWe could do something like this:\n\ndiff --git a/run-command.c b/run-command.c\nindex 8619c76..ec8a2e6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -2,6 +2,21 @@\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n \n+static void flush_one(FILE *fh) {\n+\t/* if we can't get the lock, some thread\n+\t * is already writing/flushing it */\n+\tif (ftrylockfile(fh))\n+\t\treturn;\n+\n+\tfflush(fh);\n+\tfunlockfile(fh);\n+}\n+\n+static void flush_all() {\n+\tflush_one(stdout);\n+\tflush_one(stderr);\n+}\n+\n static inline void close_pair(int fd[2])\n {\n \tclose(fd[0]);\n@@ -199,7 +214,7 @@ fail_pipe:\n \t}\n \n \ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n-\tfflush(NULL);\n+\tflush_all();\n \n #ifndef WIN32\n {\n@@ -530,7 +545,7 @@ int start_async(struct async *async)\n \n #ifdef NO_PTHREADS\n \t/* Flush stdio before fork() to avoid cloning buffers */\n-\tfflush(NULL);\n+\tflush_all();\n \n \tasync->pid = fork();\n \tif (async->pid < 0) {\n\nbut I'm not all that happy with it. It does remove the deadlock, though\nit makes the race condition for duplicate output slightly worse. The\ncomment in this hunk:\n\n+static void flush_one(FILE *fh) {\n+\t/* if we can't get the lock, some thread\n+\t * is already writing/flushing it */\n+\tif (ftrylockfile(fh))\n+\t\treturn;\n\nis a little optimistic. Somebody may be writing to the stream, but not\nenough to flush. We fail to flush, and then the forked process has the\nduplicated buffer. To be fair, this race condition already exists. If a\nthread is writing and not flushing a buffer, it may do a write after the\nfflush(NULL) but before the fork, anyway.\n\nOf slightly more concern is this hunk:\n\n+static void flush_all() {\n+\tflush_one(stdout);\n+\tflush_one(stderr);\n+}\n\nwhich obviously is not really \"all\" but just a fixed set of descriptors.\nBut AFAIK, there is no portable way to get the list of all streams (even\nthough it clearly must exist to implement fflush(NULL) properly).\n\nI wonder if that matters, though. Consider why we fflush(NULL); it is to\navoid duplicate output across a fork. But we fork in only two cases:\n\n 1. To start an async process when we don't have pthreads. But for this\n to be a problem, we must be using pthreads already.\n\n 2. To immediately exec a command. In that case, we control the whole\n code path up to the point of exec (at which point we no longer care\n about duplicated buffers), so we know which streams will be written\n to. I assumed it was just stderr, but actually I think it may be\n none at all. We replace the die routine with one that writes\n straight to the stderr descriptor.\n\nSo I am wondering if we could simply drop the fflush(NULL) entirely in\nthe start_command case. And in the start_async case, move it inside the\nNO_PTHREADS case.\n\nI guess the fflush does do one other thing; it makes sure that output on\na single descriptor is ordered sensibly. And we would be losing that.\n\nBleh. I hate parallel programming. Maybe my original fix is the right\nthing to do. It's simple and obviously correct. It does mean we might\nrun into this problem again, but we really don't use threads very much,\nso it's probably not worth spending too much up-front effort to prevent\na later coding error that is not very likely to occur.\n\nI do still wonder about the win32 deadlock that Erik mentioned. Does my\npatch help at all, or is there another bug hiding somewhere? This\nparticular deadlock only occurs with shallow clones.\n\n-Peff\n"},{"id":"165303","messageId":"BANLkTi=5WgfLoBU1ZXqyEkoBf_pQu2QKOA@mail.gmail.com","threadId":"26983","inReplyTo":"20110406175413.GA8205@sigill.intra.peff.net","subject":"Re: [RFC] upload-pack deadlock","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-06T19:15:31Z","receivedAt":"2011-04-06T19:15:31Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Apr 6, 2011 at 7:54 PM, Jeff King wrote:\n> I do still wonder about the win32 deadlock that Erik mentioned. Does my\n> patch help at all, or is there another bug hiding somewhere?\n\nNope :(\n\nIf you're interested, you can read some more information here on the\nmsysGit mailing list:\nhttp://groups.google.com/group/msysgit/browse_thread/thread/865318a3e89d9e64/9d475b8ff3cef3c3\n\nIt only happens when pushing over the git-protocol. But it happens consistently.\n"},{"id":"165314","messageId":"20110406213333.GA18481@sigill.intra.peff.net","threadId":"26983","inReplyTo":"20110406175413.GA8205@sigill.intra.peff.net","subject":"Re: [RFC] upload-pack deadlock","fromName":"Jeff King","fromEmail":"peff@github.com","sentAt":"2011-04-06T21:33:33Z","receivedAt":"2011-04-06T21:33:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 06, 2011 at 01:54:13PM -0400, Jeff King wrote:\n\n> So I am wondering if we could simply drop the fflush(NULL) entirely in\n> the start_command case. And in the start_async case, move it inside the\n> NO_PTHREADS case.\n> \n> I guess the fflush does do one other thing; it makes sure that output on\n> a single descriptor is ordered sensibly. And we would be losing that.\n\nAfter reading 13af8cb (start_command: flush buffers in the WIN32 code\npath as well, 2011-02-04), I think dropping the fflush is a bad idea.\nLet's do the simple and safe fix, and if this type of problem actually\ncomes up more than once, then I'll think about over-engineering an\nabstraction to fix it. :)\n\nHere it is with a nice commit message.\n\n-- >8 --\nSubject: [PATCH] upload-pack: start pack-objects before async rev-list\n\nIn a pthread-enabled version of upload-pack, there's a race condition\nthat can cause a deadlock on the fflush(NULL) we call from run-command.\n\nWhat happens is this:\n\n 1. Upload-pack is informed we are doing a shallow clone.\n\n 2. We call start_async() to spawn a thread that will generate rev-list\n results to feed to pack-objects. It gets a file descriptor to a\n pipe which will eventually hook to pack-objects.\n\n 3. The rev-list thread uses fdopen to create a new output stream\n around the fd we gave it, called pack_pipe.\n\n 4. The thread writes results to pack_pipe. Outside of our control,\n libc is doing locking on the stream. We keep writing until the OS\n pipe buffer is full, and then we block in write(), still holding\n the lock.\n\n 5. The main thread now uses start_command to spawn pack-objects.\n Before forking, it calls fflush(NULL) to flush every stdio output\n buffer. It blocks trying to get the lock on pack_pipe.\n\nAnd we have a deadlock. The thread will block until somebody starts\nreading from the pipe. But nobody will read from the pipe until we\nfinish flushing to the pipe.\n\nTo fix this, we swap the start order: we start the\npack-objects reader first, and then the rev-list writer\nafter. Thus the problematic fflush(NULL) happens before we\neven open the new file descriptor (and even if it didn't,\nflushing should no longer block, as the reader at the end of\nthe pipe is now active).\n\nSigned-off-by: Jeff King \n---\nAnd the result is one line shorter, so it _must_ be good.\n\n upload-pack.c | 23 +++++++++++------------\n 1 files changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex bba053f..ce5cbbe 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -157,15 +157,8 @@ static void create_pack_file(void)\n \tconst char *argv[10];\n \tint arg = 0;\n \n-\tif (shallow_nr) {\n-\t\tmemset(&rev_list, 0, sizeof(rev_list));\n-\t\trev_list.proc = do_rev_list;\n-\t\trev_list.out = -1;\n-\t\tif (start_async(&rev_list))\n-\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n-\t\targv[arg++] = \"pack-objects\";\n-\t} else {\n-\t\targv[arg++] = \"pack-objects\";\n+\targv[arg++] = \"pack-objects\";\n+\tif (!shallow_nr) {\n \t\targv[arg++] = \"--revs\";\n \t\tif (create_full_pack)\n \t\t\targv[arg++] = \"--all\";\n@@ -183,7 +176,7 @@ static void create_pack_file(void)\n \targv[arg++] = NULL;\n \n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n-\tpack_objects.in = shallow_nr ? rev_list.out : -1;\n+\tpack_objects.in = -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n@@ -192,8 +185,14 @@ static void create_pack_file(void)\n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n \n-\t/* pass on revisions we (don't) want */\n-\tif (!shallow_nr) {\n+\tif (shallow_nr) {\n+\t\tmemset(&rev_list, 0, sizeof(rev_list));\n+\t\trev_list.proc = do_rev_list;\n+\t\trev_list.out = pack_objects.in;\n+\t\tif (start_async(&rev_list))\n+\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n+\t}\n+\telse {\n \t\tFILE *pipe_fd = xfdopen(pack_objects.in, \"w\");\n \t\tif (!create_full_pack) {\n \t\t\tint i;\n-- \n1.7.4.3.13.g0b769.dirty\n"},{"id":"165316","messageId":"20110406213818.GB18481@sigill.intra.peff.net","threadId":"26983","inReplyTo":"BANLkTi=5WgfLoBU1ZXqyEkoBf_pQu2QKOA@mail.gmail.com","subject":"Re: [RFC] upload-pack deadlock","fromName":"Jeff King","fromEmail":"peff@github.com","sentAt":"2011-04-06T21:38:18Z","receivedAt":"2011-04-06T21:38:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 06, 2011 at 09:15:31PM +0200, Erik Faye-Lund wrote:\n\n> On Wed, Apr 6, 2011 at 7:54 PM, Jeff King wrote:\n> > I do still wonder about the win32 deadlock that Erik mentioned. Does my\n> > patch help at all, or is there another bug hiding somewhere?\n> \n> Nope :(\n> \n> If you're interested, you can read some more information here on the\n> msysGit mailing list:\n> http://groups.google.com/group/msysgit/browse_thread/thread/865318a3e89d9e64/9d475b8ff3cef3c3\n> \n> It only happens when pushing over the git-protocol. But it happens consistently.\n\nThanks for an interesting read. Unfortunately, my reading of that thread\nis that the bug isn't in git code, but in some weird Windows\ninteraction, and it isn't worth me trying to reproduce on Linux. So I\nguess it is probably unrelated to this.\n\n-Peff\n"},{"id":"166024","messageId":"20110418053406.GA5256@elie","threadId":"26983","inReplyTo":"20110406213333.GA18481@sigill.intra.peff.net","subject":"Re: [RFC] upload-pack deadlock","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-18T05:34:06Z","receivedAt":"2011-04-18T05:34:06Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Subject: [PATCH] upload-pack: start pack-objects before async rev-list\n>\n> In a pthread-enabled version of upload-pack, there's a race condition\n> that can cause a deadlock on the fflush(NULL) we call from run-command.\n[...]\n> To fix this, we swap the start order: we start the\n> pack-objects reader first, and then the rev-list writer\n> after. Thus the problematic fflush(NULL) happens before we\n> even open the new file descriptor (and even if it didn't,\n> flushing should no longer block, as the reader at the end of\n> the pipe is now active).\n>\n> Signed-off-by: Jeff King \n\nt5500.12 \"fetch same depth in shallow repo\" reproducibly hangs[1] on\nthe HURD without this patch and passes with it. I had just assumed it\nwas some weird hurd thing. Thanks for figuring it out.\n\nTested-by: Jonathan Nieder \n\n[1] http://bugs.debian.org/607346\n"},{"id":"168754","messageId":"20110526064547.GA18777@elie","threadId":"26983","inReplyTo":"20110406213333.GA18481@sigill.intra.peff.net","subject":"[1.7.2] Please cherry-pick \"upload-pack: start pack-objects before async rev-list\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-26T06:45:48Z","receivedAt":"2011-05-26T06:45:48Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Junio,\n\nJeff King wrote:\n\n> Subject: [PATCH] upload-pack: start pack-objects before async rev-list\n>\n> In a pthread-enabled version of upload-pack, there's a race condition\n> that can cause a deadlock on the fflush(NULL) we call from run-command.\n\nThis server-side deadlock started being triggered by shallow clones\nwhen sv.gnu.org upgraded to v1.7.2.5 a couple of months ago[1]. So it\nmight be worth thinking about how to help upgrade-averse server admins\nto fix it.\n\nLuckily, Jeff's patch (v1.7.5.1~4^2, upload-pack: start pack-objects\nbefore async rev-list, 2011-04-06) applies cleanly on top of the\noldest commit with the problem that I can find (v1.7.2-rc0~31^2~3,\nReimplement async procedures using pthreads, 2010-03-06). Could you\nqueue it for inclusion in maint-1.7.2?\n\nIf I understand correctly, Windows with threaded async rev-list before\nthen would not have suffered from this deadlock because it did not\nfflush(NULL) in start_command until v1.7.4.1~18 (start_command: flush\nbuffers in the WIN32 code path as well, 2011-02-04).\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/172042\n"},{"id":"168801","messageId":"7voc2pxutw.fsf@alter.siamese.dyndns.org","threadId":"26983","inReplyTo":"20110526064547.GA18777@elie","subject":"Re: [1.7.2] Please cherry-pick \"upload-pack: start pack-objects before async rev-list\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T16:58:19Z","receivedAt":"2011-05-26T16:58:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> This server-side deadlock started being triggered by shallow clones\n> when sv.gnu.org upgraded to v1.7.2.5 a couple of months ago[1]. So it\n> might be worth thinking about how to help upgrade-averse server admins\n> to fix it.\n\nUpgrade-averse people can be fixed by keeping them closer to the tip, no?\nI don't plan to issue any more maintenance release on 1.7.2.X track beyond\nwhat is already released, unless there is a high priority security fix or\nsomething.\n\nPlacing it on 1.7.4.X and newer maintenance tracks is a separate matter.\n"},{"id":"168804","messageId":"20110526171110.GE24931@elie","threadId":"26983","inReplyTo":"7voc2pxutw.fsf@alter.siamese.dyndns.org","subject":"Re: [1.7.2] Please cherry-pick \"upload-pack: start pack-objects before async rev-list\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-26T17:11:10Z","receivedAt":"2011-05-26T17:11:10Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> This server-side deadlock started being triggered by shallow clones\n>> when sv.gnu.org upgraded to v1.7.2.5 a couple of months ago[1]. So it\n>> might be worth thinking about how to help upgrade-averse server admins\n>> to fix it.\n>\n> Upgrade-averse people can be fixed by keeping them closer to the tip, no?\n> I don't plan to issue any more maintenance release on 1.7.2.X track beyond\n> what is already released, unless there is a high priority security fix or\n> something.\n>\n> Placing it on 1.7.4.X and newer maintenance tracks is a separate matter.\n\nOk, that's fine with me. (To be clear, I was suggesting\ncherry-picking to the branches but not making a release, though I\nrealize that would mean more time wasted the next time it is time to\nmake a high priority security fix.)\n\nI'll cherry-pick it as a Debian-specific patch, which should be good\nenough to get shallow clones working again on the affected servers.\n"}]}