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

On Thu, Apr 14, 2011 at 09:36:25PM +0200, Johannes Sixt wrote:

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

quoted

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)?

Sadly, yes, I think that is what we would have to do. It does feel like
an awful hack. And it will interact badly with things like valgrind,
which open descriptors behind the scenes (but can properly handle
the forking).

I just don't see another way around it for the general case.  The
"usual" fix for this sort of thing is that the descriptors should have
close-on-exec set, but that doesn't work for us here, because we are
only forking.

It's sufficiently ugly (and still possible to break in the pthreads
case!) that it may be worth not worrying about the general case at all,
and just fixing this one with the explicit close.

quoted

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.

Hrm, I see the code now. That seems like an odd thing to do to me.
Doesn't it disallow:

  /* set up a command */
  const char **argv = { "some", "command" };
  struct child_process c;
  c.argv = argv;
  c.out = fd;

  /* run it */
  run_command(&c);

  /* now tack our own output to the end */
  write(fd, "foo", 3);

And even weirder, we only do the close for high file descriptors. So you
_can_ do that above if "fd" is stdout, but not with an arbitrary fd.

I guess it is neither here nor there with respect to this problem; it
clearly is not something we want to do a lot, as it doesn't seem to have
come up.

But at least it explains what's going on here in the threaded case.

Yeah, from my reading of the code, you are right.

-Peff