Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error

On Donnerstag, 14. April 2011, Jeff King wrote:

Obviously it totally breaks the start_async abstraction if the called
code needs to care whether it forked or not. But we can use that to our
advantage, since it means start_async callers must assume the interface
is very limited.  So I think we can do something like:

  1. Async code declares which file descriptors it cares about. This
     would automatically include the pipe we give to it, of course.
     So the declared ones for a sideband demuxer would be stderr, and
     some network fd for reading.

  2. In the pthreads case, we do nothing. In the forked case, the child
     closes every descriptor except the "interesting" ones.

And that solves this problem, and the general case that async-callers
have no idea if they have just leaked pipe descriptors in the forked
case.

Sounds like a plan. How do you close all file descriptors? Just iterate up to 
getrlimit(RLIMIT_NOFILE)?

I'm still slightly confused, though, because I never see that descriptor
get closed in the threaded case. So I still don't understand why it
_doesn't_ deadlock with pthreads.

In the threaded case, this fd is closed by start_command(), where it is passed 
as po.out in pack_objects(). In the fork case this is too late because a 
duplicate was already inherited to the sideband demuxer.

However, pack_objects() works differently in the stateless_rpc case: then it 
does not close fd anywhere, and I think it should be possible to construct a 
similar case that hangs even in the threaded case. And the fix could simply 
look like this:

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 5e772c7..c8f601f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c

@@ -101,6 +101,7 @@ static int pack_objects(int fd, struct ref *refs,
 		free(buf);
 		close(po.out);
 		po.out = -1;
+		close(fd);
 	}
 
 	if (finish_command(&po))

-- Hannes