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

[Johannes Sixt <j6t@kdbg.org>]

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