-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Peter N. Lundblad wrote:
> 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
>
>
> ------------------------------------------------------------------------
>
> Fix bug made more visible by r13973. If a hook script put jobs in the
> background, the svn operation would block waiting for the background
> processes to finish even if the hook script itself had exited.
>
> Found by: loren jan wilson <loren@solar.uchicago.edu>
>
> * subversion/libsvn_repos/hooks.c (run_hook_cmd): Make the read and write
> ends of the stderr pipe non-inherited by child processes.
>
>
> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 18097)
> +++ subversion/libsvn_repos/hooks.c (arbetskopia)
> @@ -66,6 +66,16 @@
> return svn_error_wrap_apr
> (apr_err, _("Can't create pipe for hook '%s'"), cmd);
>
> + /* Pipes are inherited by default, but we don't want that, since
> + APR will duplicate the write end of the pipe for the child process.
> + Not closing the read end is harmless, but if the write end is inherited,
> + it will be inherited by grandchildren as well. This causes problems
> + if a hook script puts long-running jobs in the background. Even if
> + they redirect stderr to something else, the write end of our pipe will
> + still be open, causing us to block. */
> + apr_file_inherit_unset (read_errhandle);
> + apr_file_inherit_unset (write_errhandle);
> +
> /* Redirect stdout to the null device */
> apr_err = apr_file_open (&null_handle, SVN_NULL_DEVICE_NAME, APR_WRITE,
> APR_OS_DEFAULT, pool);
I like this solution, but shouldn't we be checking for error return from
those APR calls?
Max.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)
iD8DBQFDzSRbfFNSmcDyxYARAlKaAJ9loVGwItjifUaXsSYKbhVCCJzDFgCfZv0P
jNElRzfQ2TrLvjYwgk719sQ=
=S/dh
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jan 17 21:10:30 2006