@matheusbsbs / matheus-bagliano / commits / b961219779
Fork 0

upload-pack: start pack-objects before async rev-list

In a pthread-enabled version of upload-pack, there's a race condition that can cause a deadlock on the fflush(NULL) we call from run-command. What happens is this: 1. Upload-pack is informed we are doing a shallow clone. 2. We call start_async() to spawn a thread that will generate rev-list results to feed to pack-objects. It gets a file descriptor to a pipe which will eventually hook to pack-objects. 3. The rev-list thread uses fdopen to create a new output stream around the fd we gave it, called pack_pipe. 4. The thread writes results to pack_pipe. Outside of our control, libc is doing locking on the stream. We keep writing until the OS pipe buffer is full, and then we block in write(), still holding the lock. 5. The main thread now uses start_command to spawn pack-objects. Before forking, it calls fflush(NULL) to flush every stdio output buffer. It blocks trying to get the lock on pack_pipe. And we have a deadlock. The thread will block until somebody starts reading from the pipe. But nobody will read from the pipe until we finish flushing to the pipe. To fix this, we swap the start order: we start the pack-objects reader first, and then the rev-list writer after. Thus the problematic fflush(NULL) happens before we even open the new file descriptor (and even if it didn't, flushing should no longer block, as the reader at the end of the pipe is now active). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Apr 6, 2011 at 17:33 UTC b9612197798dbfc622c766e83b1fe4c20bffae5c
1 file changed +11 -12
upload-pack.c
+11 -12
@@ -156,15 +156,8 @@ static void create_pack_file(void)
156 const char *argv[10];
157 int arg = 0;
158
159 - if (shallow_nr) {
160 - memset(&rev_list, 0, sizeof(rev_list));
161 - rev_list.proc = do_rev_list;
162 - rev_list.out = -1;
163 - if (start_async(&rev_list))
164 - die("git upload-pack: unable to fork git-rev-list");
165 - argv[arg++] = "pack-objects";
166 - } else {
167 - argv[arg++] = "pack-objects";
159 + argv[arg++] = "pack-objects";
160 + if (!shallow_nr) {
161 argv[arg++] = "--revs";
162 if (create_full_pack)
163 argv[arg++] = "--all";
@@ -182,7 +175,7 @@ static void create_pack_file(void)
175 argv[arg++] = NULL;
176
177 memset(&pack_objects, 0, sizeof(pack_objects));
185 - pack_objects.in = shallow_nr ? rev_list.out : -1;
178 + pack_objects.in = -1;
179 pack_objects.out = -1;
180 pack_objects.err = -1;
181 pack_objects.git_cmd = 1;
@@ -191,8 +184,14 @@ static void create_pack_file(void)
184 if (start_command(&pack_objects))
185 die("git upload-pack: unable to fork git-pack-objects");
186
194 - /* pass on revisions we (don't) want */
195 - if (!shallow_nr) {
187 + if (shallow_nr) {
188 + memset(&rev_list, 0, sizeof(rev_list));
189 + rev_list.proc = do_rev_list;
190 + rev_list.out = pack_objects.in;
191 + if (start_async(&rev_list))
192 + die("git upload-pack: unable to fork git-rev-list");
193 + }
194 + else {
195 FILE *pipe_fd = xfdopen(pack_objects.in, "w");
196 if (!create_full_pack) {
197 int i;