Re: svn 1.3.0: post-commit hook waits even if tasks are backgrounded

On Fri, 13 Jan 2006, Ben Collins-Sussman wrote:

> Garrett Rooney and Peter Lundblad are busy discussing this problem
> right now on irc.freenode.net, #svn-dev. Seems tricky.
>
Yeah, and we've been busy all the weekend as well:-)

Seriously, the problem is that apr pipes are inherited by child processes
by default (at least on Unix). I don't know if this is a bug; I'll ask on
the APR list.

The problem this causes is that the write end of our pipe we create for
stderr will be open twice in the child process (one for stderr and then
another descriptor that was the original descriptor created for the pipe).
If the hook script creates child processes that run longer than the hook
script itself, they will keep the write end open, even if stderr is
redirected to something else. Since we, first read the stderr pipe in the
parent until EOF is reached (no writers are left) and then wait for the
hook script, we will block until the background processes finish.

Note that this problem isn't new; it is just made much more visible since
r13973. Before, we only read stderr from the hook if the process returns a
non-zero exit code. I tried something like

...
sleep 200 2> /dev/null &
exit 1

in svn 1.2.x and it also blocked, so strictly, this is not a regression.

The attached patch fixes the problem by explicitly making the pipe
descriptors non-inherited, avoiding the leaked descriptors. It still means
that a hook that puts some job in the background has to redirect stderr,
but it already had to to be safe. I think it is reasonable to have the
script author explicitly discard the stderr output.

Alternatives:
One alternative to this would be to have a temp file instead of a pipe.
Then just wait for the hook script and then read the temp file and delete
it. This avoids blocking even if the hook script doesn't redirect the
background process' stderr, but this will still have the grandchildren
holding the tempfile open and we know how problematic it can be to delete
an open file on Windows...

On IRC, it was suggested to check if the child process is still alive and
do non-blocking reads in some loop. This is complex and doesn't really
work, since we don't know when to stop reading, actually.

Any objections to commit this patch and propose it for 1.3 backport?

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org