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

[Greg Hudson <gh...@MIT.EDU>]

On Tue, 2006-01-17 at 14:41 +0100, Peter N. Lundblad wrote:
> 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).

That doesn't match my reading of the APR code.  From apr_proc_create():

        if (attr->child_err) {
            apr_file_close(attr->parent_err);
            dup2(attr->child_err->filedes, STDERR_FILENO);
            apr_file_close(attr->child_err);
        }


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

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

[Greg Hudson <gh...@MIT.EDU>]

On Tue, 2006-01-17 at 20:33 +0100, Peter N. Lundblad wrote:
> Yeah, you're not the first one to get confused. But go read
> apr_procattr_childerr_set. This function creates a new pipe, allocating
> file descriptors for its own use and then dup2s the descriptor(s) we gave
> it. The allocated descriptors will be closed after having been duplicated
> to stderr, but the descriptor we gave it will not be closed.

Okay, I understand.

APR is clearly poorly architected here, and I think you've found the
best workaround for now.  I looked for examples of httpd starting
processes with input or output streams set, and there don't seem to be
any.

(Another workaround would be to close the pipes after we set them with
apr_procattr_child_err_set and friends, but our APIs make that
difficult.)


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

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

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Tue, 17 Jan 2006, Greg Hudson wrote:

> On Tue, 2006-01-17 at 14:41 +0100, Peter N. Lundblad wrote:
> > 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).
>
> That doesn't match my reading of the APR code.  From apr_proc_create():
>
>         if (attr->child_err) {
>             apr_file_close(attr->parent_err);
>             dup2(attr->child_err->filedes, STDERR_FILENO);
>             apr_file_close(attr->child_err);
>         }
>

Yeah, you're not the first one to get confused. But go read
apr_procattr_childerr_set. This function creates a new pipe, allocating
file descriptors for its own use and then dup2s the descriptor(s) we gave
it. The allocated descriptors will be closed after having been duplicated
to stderr, but the descriptor we gave it will not be closed.

Hope this clarifies,
//Peter

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

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

[Philip Martin <ph...@codematters.co.uk>]

Greg Hudson <gh...@MIT.EDU> writes:

> On Tue, 2006-01-17 at 14:41 +0100, Peter N. Lundblad wrote:
>> 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).
>
> That doesn't match my reading of the APR code.  From apr_proc_create():
>
>         if (attr->child_err) {
>             apr_file_close(attr->parent_err);
>             dup2(attr->child_err->filedes, STDERR_FILENO);
>             apr_file_close(attr->child_err);
>         }

Subversion's run_hook_cmd() creates the pipe and then passes one end
to svn_io_start_cmd() where it gets passed to
apr_procattr_child_err_set() where it gets used as an argument to
apr_file_dup2().  The dup2 creates a duplicate file descriptor, so
there are two descriptors open before the fork() and the code shown
above only addresses one of them.

-- 
Philip Martin

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