You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Eric Gillespie ep...@pretzelnet.org on 2003/01/23 04:44:08 UTC
Problem with apr_proc_wait and/or svn_io_run_cmd
As an attempt to make this on-topic for both lists, i won't go
into how i discovered the bug. I don't think it's necessary.
Correctly using waitpid(2) involves checking for EINTR and trying
again. That leads to common usage being something like this:
while (waitpid(pid, status, 0) 0) {
if (errno != EINTR) {
break;
}
}
if (status 0) {
handle error;
} else {
success;
}
It looks like the APR equivalent is supposed to be:
do {
apr_err = apr_proc_wait (cmd_proc, exitcode_val,
exitwhy_val, APR_WAIT);
} while (APR_STATUS_IS_CHILD_NOTDONE (apr_err));
Is that correct? If so, svn_io_run_cmd needs to be fixed to do
that. Currently it only tries once:
/* Wait for the cmd command to finish. */
apr_err = apr_proc_wait (cmd_proc, exitcode_val,
exitwhy_val, APR_WAIT);
if (APR_STATUS_IS_CHILD_NOTDONE (apr_err))
return svn_error_createf
(apr_err, NULL,
svn_io_run_cmd: error waiting for %s process,
cmd);
Furthermore, apr_proc_wait itself has a problem:
if ((pstatus = waitpid(proc-pid, exit_int, waitpid_options)) 0) {
(handle success, i skip this because it is fine)
...
}
else if (pstatus == 0) {
return APR_CHILD_NOTDONE;
}
return errno;
}
That's it. It never handles the case where waitpid(2) returns
-1. But maybe it doesn't need to? Maybe the apr_proc_wait
caller is expected to check for EINTR? But that means the APR
idiom needs to be:
do {
apr_err = apr_proc_wait (cmd_proc, exitcode_val,
exitwhy_val, APR_WAIT);
} while (APR_STATUS_IS_CHILD_NOTDONE (apr_err) || apr_err == EINTR);
Or maybe APR_STATUS_IS_CHILD_NOTDONE should handle that? I don't
know, but *something* needs to change, because currently
svn_io_run_cmd breaks when waitpid(2) is interrupted (which turns
out to be the root of a problem i have been debugging tonight).
It may be as simple as changing APR_STATUS_IS_CHILD_NOTDONE, in
which case apr_proc_wait doesn't need to change at all. But i am
not sure that is the solution. No matter what, svn_io_run_cmd
will need to change so that it repeats the apr_proc_wait call as
necessary (unless you want to make apr_proc_wait itself loop over
waitpid(2), which i think is NOT the way to go).
--
Eric Gillespie, Jr. * epg@pretzelnet.org
Build a fire for a man, and he'll be warm for a day. Set a man on
fire, and he'll be warm for the rest of his life. -Terry Pratchett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Eric Gillespie ep...@pretzelnet.org.
=?UTF-8?B?QnJhbmtvIMSMaWJlag==?= brane@xbc.nu writes:
Why not? Interrupted system calls are a very Unixy thing, and can't even
be simulated on Windows, for example. It's very unfriendly to have to
loop on something that's supposed to be a blocking call. I think the
Unix implementation should handle EINTR internally. That's the only way
to make the behaviour the same on all platforms.
Ah, forgive me for being unix-centric. You may have a point.
--
Eric Gillespie * epg@pretzelnet.org
Build a fire for a man, and he'll be warm for a day. Set a man on
fire, and he'll be warm for the rest of his life. -Terry Pratchett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Eric Gillespie <ep...@pretzelnet.org>.
=?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <br...@xbc.nu> writes:
> Why not? Interrupted system calls are a very Unixy thing, and can't even
> be simulated on Windows, for example. It's very unfriendly to have to
> loop on something that's supposed to be a blocking call. I think the
> Unix implementation should handle EINTR internally. That's the only way
> to make the behaviour the same on all platforms.
Ah, forgive me for being unix-centric. You may have a point.
--
Eric Gillespie <*> epg@pretzelnet.org
Build a fire for a man, and he'll be warm for a day. Set a man on
fire, and he'll be warm for the rest of his life. -Terry Pratchett
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Branko Čibej br...@xbc.nu.
Eric Gillespie wrote:
[snip]
It may be as simple as changing APR_STATUS_IS_CHILD_NOTDONE, in
which case apr_proc_wait doesn't need to change at all. But i am
not sure that is the solution. No matter what, svn_io_run_cmd
will need to change so that it repeats the apr_proc_wait call as
necessary (unless you want to make apr_proc_wait itself loop over
waitpid(2), which i think is NOT the way to go).
Why not? Interrupted system calls are a very Unixy thing, and can't even
be simulated on Windows, for example. It's very unfriendly to have to
loop on something that's supposed to be a blocking call. I think the
Unix implementation should handle EINTR internally. That's the only way
to make the behaviour the same on all platforms.
--
Brane Čibej brane@xbc.nu http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Sander Roobol <ph...@wanadoo.nl>.
On Wed, Mar 05, 2003 at 05:49:28PM +0000, Thom May wrote:
> * Sander Roobol (phy@wanadoo.nl) wrote :
> > So... what happend to this? It looks to me that the patch didn't made it
> > into APR, so my question is, has this issue (whatever it might be) been
> > addressed in Subversion? (I'm intentionally not Cc'ing the APR list
> > because I don't think they care about this.)
> >
> It's still on my radar for APR; I need to get the httpd test suite set up
> (just got back from holiday) so I can confirm that it doesn't break anything
> in that project and then I'll be pretty happy to commit it...
> -Thom
I filed issue #1234 for this problem.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Thom May <th...@planetarytramp.net>.
* Sander Roobol (phy@wanadoo.nl) wrote :
> On Thu, Jan 23, 2003 at 08:47:09PM -0500, Eric Gillespie wrote:
> >
> > OK, i'm not sure what you're talking about with the "xyy
> > specials" (i am not that familiar with APR), but the following
> > patch solves the problem i reported. The Subversion test suite
> > now passess completely with ra_svn after applying this patch to
> > APR.
> >
> > CCing the Subversion list again because if this patch is accepted
> > Subversion will NOT need to be changed.
>
> So... what happend to this? It looks to me that the patch didn't made it
> into APR, so my question is, has this issue (whatever it might be) been
> addressed in Subversion? (I'm intentionally not Cc'ing the APR list
> because I don't think they care about this.)
>
It's still on my radar for APR; I need to get the httpd test suite set up
(just got back from holiday) so I can confirm that it doesn't break anything
in that project and then I'll be pretty happy to commit it...
-Thom
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Sander Roobol <ph...@wanadoo.nl>.
On Thu, Jan 23, 2003 at 08:47:09PM -0500, Eric Gillespie wrote:
> Jeff Trawick <tr...@attglobal.net> writes:
>
> > I can live with that :)
> >
> > (yes, I'm psychologically damaged by some of the xyy specials that cutified
> > something in APR but chopped off interesting Unix-specific code at the
> > knees)
>
> OK, i'm not sure what you're talking about with the "xyy
> specials" (i am not that familiar with APR), but the following
> patch solves the problem i reported. The Subversion test suite
> now passess completely with ra_svn after applying this patch to
> APR.
>
> CCing the Subversion list again because if this patch is accepted
> Subversion will NOT need to be changed.
So... what happend to this? It looks to me that the patch didn't made it
into APR, so my question is, has this issue (whatever it might be) been
addressed in Subversion? (I'm intentionally not Cc'ing the APR list
because I don't think they care about this.)
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Eric Gillespie <ep...@pretzelnet.org>.
Jeff Trawick <tr...@attglobal.net> writes:
> I can live with that :)
>
> (yes, I'm psychologically damaged by some of the xyy specials that cutified
> something in APR but chopped off interesting Unix-specific code at the
> knees)
OK, i'm not sure what you're talking about with the "xyy
specials" (i am not that familiar with APR), but the following
patch solves the problem i reported. The Subversion test suite
now passess completely with ra_svn after applying this patch to
APR.
CCing the Subversion list again because if this patch is accepted
Subversion will NOT need to be changed.
Index: proc.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/unix/proc.c,v
retrieving revision 1.62
diff -a -u -r1.62 proc.c
--- proc.c 6 Jan 2003 23:44:38 -0000 1.62
+++ proc.c 24 Jan 2003 01:43:42 -0000
@@ -474,7 +474,11 @@
waitpid_options |= WNOHANG;
}
- if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
+ do {
+ pstatus = waitpid(proc->pid, &exit_int, waitpid_options);
+ } while (pstatus < 0 && errno == EINTR);
+
+ if (pstatus > 0) {
proc->pid = pstatus;
if (WIFEXITED(exit_int)) {
[PATCH] Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Eric Gillespie ep...@pretzelnet.org.
Jeff Trawick trawick@attglobal.net writes:
I can live with that :)
(yes, I'm psychologically damaged by some of the xyy specials that cutified
something in APR but chopped off interesting Unix-specific code at the
knees)
OK, i'm not sure what you're talking about with the xyy
specials (i am not that familiar with APR), but the following
patch solves the problem i reported. The Subversion test suite
now passess completely with ra_svn after applying this patch to
APR.
CCing the Subversion list again because if this patch is accepted
Subversion will NOT need to be changed.
Index: proc.c
===================================================================
RCS file: /home/cvspublic/apr/threadproc/unix/proc.c,v
retrieving revision 1.62
diff -a -u -r1.62 proc.c
--- proc.c 6 Jan 2003 23:44:38 -0000 1.62
+++ proc.c 24 Jan 2003 01:43:42 -0000
@@ -474,7 +474,11 @@
waitpid_options |= WNOHANG;
}
- if ((pstatus = waitpid(proc-pid, exit_int, waitpid_options)) 0) {
+ do {
+ pstatus = waitpid(proc-pid, exit_int, waitpid_options);
+ } while (pstatus 0 errno == EINTR);
+
+ if (pstatus 0) {
proc-pid = pstatus;
if (WIFEXITED(exit_int)) {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Jeff Trawick <tr...@attglobal.net>.
Cliff Woolley wrote:
> On Thu, 23 Jan 2003, Jeff Trawick wrote:
>
>
> >most but not all of the rest of APR does that
> >the inconsistency is ugly, but being able to see EINTR is important
> >functionality
>
>
> MHO: Sounds like a job for an _ex() function... apr_proc_wait() should not
> return EINTR. Another function with a similiar name (like
> apr_proc_wait_ex()) can be used if you want to get EINTR.
I can live with that :)
(yes, I'm psychologically damaged by some of the xyy specials that cutified
something in APR but chopped off interesting Unix-specific code at the
knees)
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Cliff Woolley <jw...@virginia.edu>.
On Thu, 23 Jan 2003, Jeff Trawick wrote:
> most but not all of the rest of APR does that
> the inconsistency is ugly, but being able to see EINTR is important
> functionality
MHO: Sounds like a job for an _ex() function... apr_proc_wait() should not
return EINTR. Another function with a similiar name (like
apr_proc_wait_ex()) can be used if you want to get EINTR.
--Cliff
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Jeff Trawick <tr...@attglobal.net>.
Joe Orton wrote:
> On Wed, Jan 22, 2003 at 11:44:08PM -0500, Eric Gillespie wrote:
>
> >As an attempt to make this on-topic for both lists, i won't go
> >into how i discovered the bug. I don't think it's necessary.
> >
> >Correctly using waitpid(2) involves checking for EINTR and trying
> >again. That leads to common usage being something like this:
>
>
> That does look like a bug.
>
> ...
>
> >It looks like the APR equivalent is supposed to be:
> >
> > do {
> > apr_err = apr_proc_wait (&cmd_proc, &exitcode_val,
> > &exitwhy_val, APR_WAIT);
> > } while (APR_STATUS_IS_CHILD_NOTDONE (apr_err));
> >
> >Is that correct?
>
>
> I don't think so really: the child will only be "NOTDONE" if you pass in
> APR_NOWAIT; otherwise, the waitpid waits until the child *is* done.
>
> ...
>
> >It may be as simple as changing APR_STATUS_IS_CHILD_NOTDONE, in
> >which case apr_proc_wait doesn't need to change at all. But i am
> >not sure that is the solution. No matter what, svn_io_run_cmd
> >will need to change so that it repeats the apr_proc_wait call as
> >necessary (unless you want to make apr_proc_wait itself loop over
> >waitpid(2), which i think is NOT the way to go).
>
>
> My guess would be to make apr_proc_wait loop over waitpid whilst
> errno=EINTR; that's what the rest of APR does. Though the interactions
> between waitpid and SIGCHLD here are a bit daunting, just from reading
> the Linux waitpid() man page.
most but not all of the rest of APR does that
the inconsistency is ugly, but being able to see EINTR is important
functionality
it can mean that certain apps can have simple signal handlers that only
have to set flags instead of having to longjmp out to avoid the APR
"while (EINTR)" loop
it can mean that certain apps have better performance because they didn't
have to add extra signalling mechanisms (e.g., extra pipe to poll on) since
a signal can break the app out of apr_accept() and allow the app to
check flags
non-portability sucks, but so does losing the ability to use key Unix
features... cute doesn't matter if your program can't take advantage of
the design of the underlying system
compromise may be some attribute on APR object or flag on certain
APR functions to tell APR to surface the EINTR condition
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Joe Orton jo...@manyfish.co.uk.
On Wed, Jan 22, 2003 at 11:44:08PM -0500, Eric Gillespie wrote:
As an attempt to make this on-topic for both lists, i won't go
into how i discovered the bug. I don't think it's necessary.
Correctly using waitpid(2) involves checking for EINTR and trying
again. That leads to common usage being something like this:
That does look like a bug.
...
It looks like the APR equivalent is supposed to be:
do {
apr_err = apr_proc_wait (cmd_proc, exitcode_val,
exitwhy_val, APR_WAIT);
} while (APR_STATUS_IS_CHILD_NOTDONE (apr_err));
Is that correct?
I don't think so really: the child will only be NOTDONE if you pass in
APR_NOWAIT; otherwise, the waitpid waits until the child *is* done.
...
It may be as simple as changing APR_STATUS_IS_CHILD_NOTDONE, in
which case apr_proc_wait doesn't need to change at all. But i am
not sure that is the solution. No matter what, svn_io_run_cmd
will need to change so that it repeats the apr_proc_wait call as
necessary (unless you want to make apr_proc_wait itself loop over
waitpid(2), which i think is NOT the way to go).
My guess would be to make apr_proc_wait loop over waitpid whilst
errno=EINTR; that's what the rest of APR does. Though the interactions
between waitpid and SIGCHLD here are a bit daunting, just from reading
the Linux waitpid() man page.
Regards,
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Branko Čibej <br...@xbc.nu>.
Eric Gillespie wrote:
[snip]
>It may be as simple as changing APR_STATUS_IS_CHILD_NOTDONE, in
>which case apr_proc_wait doesn't need to change at all. But i am
>not sure that is the solution. No matter what, svn_io_run_cmd
>will need to change so that it repeats the apr_proc_wait call as
>necessary (unless you want to make apr_proc_wait itself loop over
>waitpid(2), which i think is NOT the way to go).
>
Why not? Interrupted system calls are a very Unixy thing, and can't even
be simulated on Windows, for example. It's very unfriendly to have to
loop on something that's supposed to be a blocking call. I think the
Unix implementation should handle EINTR internally. That's the only way
to make the behaviour the same on all platforms.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
Re: Problem with apr_proc_wait and/or svn_io_run_cmd
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Wed, Jan 22, 2003 at 11:44:08PM -0500, Eric Gillespie wrote:
> As an attempt to make this on-topic for both lists, i won't go
> into how i discovered the bug. I don't think it's necessary.
>
> Correctly using waitpid(2) involves checking for EINTR and trying
> again. That leads to common usage being something like this:
That does look like a bug.
...
> It looks like the APR equivalent is supposed to be:
>
> do {
> apr_err = apr_proc_wait (&cmd_proc, &exitcode_val,
> &exitwhy_val, APR_WAIT);
> } while (APR_STATUS_IS_CHILD_NOTDONE (apr_err));
>
> Is that correct?
I don't think so really: the child will only be "NOTDONE" if you pass in
APR_NOWAIT; otherwise, the waitpid waits until the child *is* done.
...
> It may be as simple as changing APR_STATUS_IS_CHILD_NOTDONE, in
> which case apr_proc_wait doesn't need to change at all. But i am
> not sure that is the solution. No matter what, svn_io_run_cmd
> will need to change so that it repeats the apr_proc_wait call as
> necessary (unless you want to make apr_proc_wait itself loop over
> waitpid(2), which i think is NOT the way to go).
My guess would be to make apr_proc_wait loop over waitpid whilst
errno=EINTR; that's what the rest of APR does. Though the interactions
between waitpid and SIGCHLD here are a bit daunting, just from reading
the Linux waitpid() man page.
Regards,
joe