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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html