You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Jeff Trawick (httpd)" <tr...@locus.apache.org> on 2000/06/16 20:10:19 UTC
extra syscalls related to ap_create_pipe()
ap_create_pipe() always makes the pipe non-blocking
- cost on Unix: 2*fntl()
- cost on Win32: 0?
(It is unclear to me that it is really created non-blocking by the
system; I suspect that it is a bug hidden by the fact that
most/all callers of ap_create_pipe() then call ap_block_pipe().)
- cost on OS/2: 0; DosCreateNPipe() has a flag, so no extra syscall
Caller 1 of ap_create_pipe(): ap_open_piped_log()
This then calls ap_block_pipe() on both ends of the pipe, at a
cost of more syscalls.
Caller 2: ap_setprocattr_io()
This calls ap_block_pipe() on at least one end of pipe. Also, it
has some fun buglets which result in even more syscalls, but I'm not
fussing about that here.
See the buglet? (missing break statements) The buglet is
repeated for out and err for good measure.
switch (in) {
case APR_FULL_BLOCK:
ap_block_pipe(attr->child_in);
ap_block_pipe(attr->parent_in);
case APR_PARENT_BLOCK:
ap_block_pipe(attr->parent_in);
case APR_CHILD_BLOCK:
ap_block_pipe(attr->child_in);
}
Some callers of ap_setprocattr_io() pass APR_FULL_BLOCK; others
pass APR_CHILD_BLOCK.
Caller 3: ap_setprocattr_childin()
called only from http_log.c for reliable piped logs, which doesn't
play with the blocking state; I'm not sure if that is intended
Caller 4: ap_setprocattr_childout()
never called
Caller 5: ap_setprocattr_childerr()
never called
My suggestions:
a) Let pipes default to blocking. Add new APR function
ap_nonblock_pipe() for callers that want them non-blocking.
or b) Add a flag to ap_create_pipe() with values APR_PIPE_BLOCKING or
APR_PIPE_NONBLOCKING. (I don't think we need different values
for each end of the pipe.) Do the right thing inside. This helps
OS/2 in particular since the pipe can be created as desired with
no extra syscall.
I prefer b). It will be clearer what is happening when you see a call
to ap_create_pipe().
ap_nonblock_pipe() may be needed anyway, but I'd like to avoid it for
now.
I don't know how much this will speed up mod_cgi vs. mod_cgid...
Probably not much, since fcntl should be pretty cheap. It will be
cleaner and traces of mod_cgi won't be quite so disturbing.
Any comments? Any preferences for a) vs. b) vs. your ideas?
Thanks,
Jeff
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
Another reason not to unconditionally make pipes non-blocking:
Win9x can't do non-blocking pipes...
Currently, with OS/2*, Unix, and I guess BeOS, ap_create_pipe()
returns handles which work non-blocking.
With WinNT you get blocking handles, but this is easily remedied.
With Win9x, there is no remedy AFAIK other than reimplementing pipes
from the ground up. I think Win9x has the old OS/2 1.x (non-server
limitation of not being able to *create* a named pipe; thus that
avenue is probably blocked.
*I guess there is magic that makes the write output handle (created
by DosOpen()) non-blocking by virtue of the fact that the pipe was
created non-blocking. Not real sure... The input handle is
definitely non-blocking.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Bill Stoddard <st...@raleigh.ibm.com>.
> > -ap_status_t ap_create_pipe(ap_file_t **in, ap_file_t **out, ap_pool_t *cont)
> > +ap_status_t ap_create_pipe(ap_file_t **in, int input_blocking,
> > + ap_file_t **out, int output_blocking,
> > + ap_pool_t *cont)
>
> I seriously dislike this new API. I also do not think it is
> necessary. Create all pipes as blocking. When somebody specifically sets
> a pipe non-blocking or sets a timeout switch it. What we have done here
> is to add complexity to an API that used to be very clean. The
> (input|output)_blocking parameters are also integers, and this leads to
> messy-ness later. See below.
>
I agree with Ryan here. I prefer the simple API.
Is it important that on some platforms, setting a timeout on a pipe may or may not set the
pipe to non-blocking? Windows NT and Unix will behave the same I believe, that is, setting
a non-zero timeout will set the pipe to non-blocking (unix will use a select to wait for
i/o and Windows will use WaitForSingleObject() or one of the selects available to it).
Other platforms (I have no examples right off hand) may be able to do something like a
setpipeopt to timeout a -blocking- call. This is how socket timeouts on Windows work (i.e,
use setsockopt(RCVTIMEO), et, al.). The Windows socket remains a blocking socket but with
a timeout built in.
Bill
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
> From: "Bill Stoddard" <st...@raleigh.ibm.com>
> Date: Tue, 20 Jun 2000 12:41:18 -0400
>
> > > O.k. I'll keep the ap_create_pipe() signature like it was, make both
> > > ends of the pipe blocking, and add ap_unblock_pipe() for when the
> > > caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
> > > the externalization of the internal function pipenonblock().
> >
> > Yes.
> >
>
> Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> sockets (see Dean's saferead code).
>
> ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
>
> Bill
For the record, either way is o.k. with me. The single function
ap_set_pipe_timeout() is sufficient for playing with timeouts and the
blocking state. (Yes, this is one and the same in the current Unix
implementation, but the app may not view it that way and it may not be
so intertwined in the implementation of timeouts on other platforms.)
If we keep ap_block_pipe(), I think ap_set_pipe_timeout(pipe, -1)
should call ap_block_pipe() under the covers.
If we keep the notion of ap_unblock_pipe(), I think
ap_set_pipe_timeout(pipe, 0) should call ap_unblock_pipe() under the
covers.
Note that some of the implementations of ap_set_pipe_timeout() don't
do the right thing with the blocking state when the timeout is
disabled. I may wait to work on this until *after* the big
all-platform commit of the change to ap_create_pipe().
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
> > > Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> > > sockets (see Dean's saferead code).
> > >
> > > ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> > > ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> > > ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
> >
> > This is how the code is supposed to work right now. This requires the
> > fcntl calls that we are trying to get rid of.
>
> I don't understand "this requires the fcntl calls..." statement. I
> would like to get rid
> of ap_block_pipe() and ap_unblock_pipe() and use
> ap_set_pipe_timeout() as documented
> above. This doesn't need extra fcntl calls (provided you maintain the
> knowledge of the
> pipe blocking state in ap_file_t).
First of all, could you please set your line break at less than 76
characters?
Okay, I see how this would work now. I don't have any really strong
opinions one way or tother. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Bill Stoddard <st...@raleigh.ibm.com>.
> > Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> > sockets (see Dean's saferead code).
> >
> > ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> > ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> > ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
>
> This is how the code is supposed to work right now. This requires the
> fcntl calls that we are trying to get rid of.
>
I don't understand "this requires the fcntl calls..." statement. I would like to get rid
of ap_block_pipe() and ap_unblock_pipe() and use ap_set_pipe_timeout() as documented
above. This doesn't need extra fcntl calls (provided you maintain the knowledge of the
pipe blocking state in ap_file_t).
Bill
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
> Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> sockets (see Dean's saferead code).
>
> ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
This is how the code is supposed to work right now. This requires the
fcntl calls that we are trying to get rid of.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Bill Stoddard <st...@raleigh.ibm.com>.
> > O.k. I'll keep the ap_create_pipe() signature like it was, make both
> > ends of the pipe blocking, and add ap_unblock_pipe() for when the
> > caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
> > the externalization of the internal function pipenonblock().
>
> Yes.
>
Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
sockets (see Dean's saferead code).
ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
Bill
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
> > No good. This needs to be moved inside the #if...#else...#endif. There
> > is a case where BeOS can not set the pipe to non-blocking mode. This
> > makes it look like the pipe can have a timeout and it can't.
>
> I see... I think what should happen is that if we're BEOS but not
> BEOS_BONE then pipenonblock() should return APR_ENOTIMPL.
>
> Here is what happens with the current CVS code...
>
> create a pipe:
>
> old BEOS:
> pipe returned in blocking state
> everywhere else
> pipe returned in non-blocking state
Or an error :-)
> set pipe timeout:
> everywhere:
> no error
>
> read from pipe when no data avail:
> old BEOS:
> hang
> everywhere else:
> return after timeout
>
> If pipenonblock() returns APR_ENOTIMPL when called from
> ap_set_pipe_timeout(), ap_set_pipe_timeout() will be able to tell the
> caller that we can't do that.
YES!!!!! Perfect.
> > I seriously dislike this new API. I also do not think it is
> > necessary. Create all pipes as blocking. When somebody specifically sets
> > a pipe non-blocking or sets a timeout switch it. What we have done here
> > is to add complexity to an API that used to be very clean. The
> > (input|output)_blocking parameters are also integers, and this leads to
> > messy-ness later. See below.
>
> O.k. I'll keep the ap_create_pipe() signature like it was, make both
> ends of the pipe blocking, and add ap_unblock_pipe() for when the
> caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
> the externalization of the internal function pipenonblock().
Yes.
> > > + child_blocking = in == APR_FULL_BLOCK || in == APR_CHILD_BLOCK;
> > > + parent_blocking = in == APR_FULL_BLOCK || in == APR_PARENT_BLOCK;
> > > +
> > > + if ((status = ap_create_pipe(&attr->child_in, child_blocking,
> > > + &attr->parent_in, parent_blocking,
> > > + attr->cntxt)) != APR_SUCCESS) {
> > > return status;
> > > }
> > > - switch (in) {
> > > - case APR_FULL_BLOCK:
> > > - ap_block_pipe(attr->child_in);
> > > - ap_block_pipe(attr->parent_in);
> > > - case APR_PARENT_BLOCK:
> > > - ap_block_pipe(attr->parent_in);
> > > - case APR_CHILD_BLOCK:
> > > - ap_block_pipe(attr->child_in);
> > > - }
> > > }
> >
> > This whole section can be pretty much left alone if we remove those two
> > parameters. I think that makes the code much easier to read.
>
> The style will remain the same; we'll just call ap_unblock_pipe() as
> follows:
>
> switch(in) {
> case APR_FULL_BLOCK:
> break;
> case APR_PARENT_BLOCK:
> ap_unblock_pipe(attr->child_in);
> break;
> case APR_CHILD_BLOCK:
> ap_unblock_pipe(attr->parent_in);
> break;
> default:
> ap_unblock_pipe(attr->child_in);
> ap_unblock_pipe(attr->parent_in);
> }
Perfect. With these changes, you have my unequivicable ++1. Please
commit when ready.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
> From: rbb@covalent.net
> Date: Tue, 20 Jun 2000 07:41:21 -0700 (PDT)
>
> > Index: src/lib/apr/file_io/unix/pipe.c
> > ===================================================================
> > RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/pipe.c,v
> > retrieving revision 1.34
> > diff -u -r1.34 pipe.c
> > --- src/lib/apr/file_io/unix/pipe.c 2000/06/17 17:04:16 1.34
> > +++ src/lib/apr/file_io/unix/pipe.c 2000/06/18 18:25:56
> > @@ -82,6 +82,8 @@
> > #endif /* BEOS_BONE */
> >
> > #endif /* !BEOS */
> > +
> > + thepipe->blocking = BLK_OFF;
> > return APR_SUCCESS;
> > }
>
> No good. This needs to be moved inside the #if...#else...#endif. There
> is a case where BeOS can not set the pipe to non-blocking mode. This
> makes it look like the pipe can have a timeout and it can't.
I see... I think what should happen is that if we're BEOS but not
BEOS_BONE then pipenonblock() should return APR_ENOTIMPL.
Here is what happens with the current CVS code...
create a pipe:
old BEOS:
pipe returned in blocking state
everywhere else
pipe returned in non-blocking state
set pipe timeout:
everywhere:
no error
read from pipe when no data avail:
old BEOS:
hang
everywhere else:
return after timeout
If pipenonblock() returns APR_ENOTIMPL when called from
ap_set_pipe_timeout(), ap_set_pipe_timeout() will be able to tell the
caller that we can't do that.
>
> I seriously dislike this new API. I also do not think it is
> necessary. Create all pipes as blocking. When somebody specifically sets
> a pipe non-blocking or sets a timeout switch it. What we have done here
> is to add complexity to an API that used to be very clean. The
> (input|output)_blocking parameters are also integers, and this leads to
> messy-ness later. See below.
O.k. I'll keep the ap_create_pipe() signature like it was, make both
ends of the pipe blocking, and add ap_unblock_pipe() for when the
caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
the externalization of the internal function pipenonblock().
> > + child_blocking = in == APR_FULL_BLOCK || in == APR_CHILD_BLOCK;
> > + parent_blocking = in == APR_FULL_BLOCK || in == APR_PARENT_BLOCK;
> > +
> > + if ((status = ap_create_pipe(&attr->child_in, child_blocking,
> > + &attr->parent_in, parent_blocking,
> > + attr->cntxt)) != APR_SUCCESS) {
> > return status;
> > }
> > - switch (in) {
> > - case APR_FULL_BLOCK:
> > - ap_block_pipe(attr->child_in);
> > - ap_block_pipe(attr->parent_in);
> > - case APR_PARENT_BLOCK:
> > - ap_block_pipe(attr->parent_in);
> > - case APR_CHILD_BLOCK:
> > - ap_block_pipe(attr->child_in);
> > - }
> > }
>
> This whole section can be pretty much left alone if we remove those two
> parameters. I think that makes the code much easier to read.
The style will remain the same; we'll just call ap_unblock_pipe() as
follows:
switch(in) {
case APR_FULL_BLOCK:
break;
case APR_PARENT_BLOCK:
ap_unblock_pipe(attr->child_in);
break;
case APR_CHILD_BLOCK:
ap_unblock_pipe(attr->parent_in);
break;
default:
ap_unblock_pipe(attr->child_in);
ap_unblock_pipe(attr->parent_in);
}
Thanks,
Jeff
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
I dislike how the patch was executed, but the idea behind it is
sound. Comments are inline.
> 1) If the caller sets a pipe blocking and later tries to set a
> timeout, the timeout won't work because we don't realize that the
> pipe is blocking, and ap_set_pipe_timeout() assumes that the pipe
> is in non-blocking.
We have a flag in the file record now, so we do know that the pipe is
blocking, and a timeout can't work. If somebody sets a pipe to block, and
then sets a timeout, we need to set the pipe non-blocking. If the flag is
set to UNKNOWN, we can treat it like it is blocking, because setting it
twice only costs us an extra syscall. I am willing to pay that price in
this case.
> 2) If the caller creates the ap_file_t via ap_put_os_file(), APR
> doesn't know whether or not the pipe is blocking, so
> ap_set_pipe_timeout() is broken again.
This is what I was talking about with it not working with
ap_get_os_file. That should have been ap_put_os_file.
> @@ -213,6 +212,7 @@
> }
> (*file)->eof_hit = 0;
> (*file)->buffered = 0;
> + (*file)->blocking = BLK_UNKNOWN; /* in case it is a pipe */
++1! This is GREAT!
> Index: src/lib/apr/file_io/unix/pipe.c
> ===================================================================
> RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/pipe.c,v
> retrieving revision 1.34
> diff -u -r1.34 pipe.c
> --- src/lib/apr/file_io/unix/pipe.c 2000/06/17 17:04:16 1.34
> +++ src/lib/apr/file_io/unix/pipe.c 2000/06/18 18:25:56
> @@ -82,6 +82,8 @@
> #endif /* BEOS_BONE */
>
> #endif /* !BEOS */
> +
> + thepipe->blocking = BLK_OFF;
> return APR_SUCCESS;
> }
No good. This needs to be moved inside the #if...#else...#endif. There
is a case where BeOS can not set the pipe to non-blocking mode. This
makes it look like the pipe can have a timeout and it can't.
> -ap_status_t ap_create_pipe(ap_file_t **in, ap_file_t **out, ap_pool_t *cont)
> +ap_status_t ap_create_pipe(ap_file_t **in, int input_blocking,
> + ap_file_t **out, int output_blocking,
> + ap_pool_t *cont)
I seriously dislike this new API. I also do not think it is
necessary. Create all pipes as blocking. When somebody specifically sets
a pipe non-blocking or sets a timeout switch it. What we have done here
is to add complexity to an API that used to be very clean. The
(input|output)_blocking parameters are also integers, and this leads to
messy-ness later. See below.
> fprintf(stdout, "\tCreating pipes.......");
> - if (ap_create_pipe(&readp, &writep, context) != APR_SUCCESS) {
> + if (ap_create_pipe(&readp, 1, &writep, 1, context) != APR_SUCCESS) {
What are those two 1's doing there? I know, and you know, but they are
really not obvious. Anybody who looks at this code later is going to be
VERY confused. APR has tried to get rid of most flags by using enums and
#defines instead of just 1, 2, or 3. Also, as I said above, I do not
think those parameters are necessary.
> + child_blocking = in == APR_FULL_BLOCK || in == APR_CHILD_BLOCK;
> + parent_blocking = in == APR_FULL_BLOCK || in == APR_PARENT_BLOCK;
> +
> + if ((status = ap_create_pipe(&attr->child_in, child_blocking,
> + &attr->parent_in, parent_blocking,
> + attr->cntxt)) != APR_SUCCESS) {
> return status;
> }
> - switch (in) {
> - case APR_FULL_BLOCK:
> - ap_block_pipe(attr->child_in);
> - ap_block_pipe(attr->parent_in);
> - case APR_PARENT_BLOCK:
> - ap_block_pipe(attr->parent_in);
> - case APR_CHILD_BLOCK:
> - ap_block_pipe(attr->child_in);
> - }
> }
> if (out) {
> - if ((status = ap_create_pipe(&attr->parent_out, &attr->child_out,
> - attr->cntxt)) != APR_SUCCESS) {
> + child_blocking = out == APR_FULL_BLOCK || out == APR_CHILD_BLOCK;
> + parent_blocking = out == APR_FULL_BLOCK || out == APR_PARENT_BLOCK;
> +
> + if ((status = ap_create_pipe(&attr->parent_out, parent_blocking,
> + &attr->child_out, child_blocking,
> + attr->cntxt)) != APR_SUCCESS) {
> return status;
> }
> - switch (out) {
> - case APR_FULL_BLOCK:
> - ap_block_pipe(attr->child_out);
> - ap_block_pipe(attr->parent_out);
> - case APR_PARENT_BLOCK:
> - ap_block_pipe(attr->parent_out);
> - case APR_CHILD_BLOCK:
> - ap_block_pipe(attr->child_out);
> - }
> }
> if (err) {
> - if ((status = ap_create_pipe(&attr->parent_err, &attr->child_err,
> - attr->cntxt)) != APR_SUCCESS) {
> + child_blocking = err == APR_FULL_BLOCK || err == APR_CHILD_BLOCK;
> + parent_blocking = err == APR_FULL_BLOCK || err == APR_PARENT_BLOCK;
> + if ((status = ap_create_pipe(&attr->parent_err, parent_blocking,
> + &attr->child_err, child_blocking,
> + attr->cntxt)) != APR_SUCCESS) {
> return status;
> }
> - switch (err) {
> - case APR_FULL_BLOCK:
> - ap_block_pipe(attr->child_err);
> - ap_block_pipe(attr->parent_err);
> - case APR_PARENT_BLOCK:
> - ap_block_pipe(attr->parent_err);
> - case APR_CHILD_BLOCK:
> - ap_block_pipe(attr->child_err);
> - }
> }
> return APR_SUCCESS;
> }
This whole section can be pretty much left alone if we remove those two
parameters. I think that makes the code much easier to read.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
[PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
Here is a new patch, fixing the timeout breakage Ryan described
yesterday. In addition, it fixes a couple of other broken scenarios
with blocking state:
1) If the caller sets a pipe blocking and later tries to set a
timeout, the timeout won't work because we don't realize that the
pipe is blocking, and ap_set_pipe_timeout() assumes that the pipe
is in non-blocking.
2) If the caller creates the ap_file_t via ap_put_os_file(), APR
doesn't know whether or not the pipe is blocking, so
ap_set_pipe_timeout() is broken again.
Regarding Ryan's earlier concern with the solution employed:
> The second method has the unfortunate
>problem of not really working with ap_get_os_file well.
I have no clue what he is alluding to. It is true that the caller of
ap_get_os_file() has no way to find out whether or not the pipe is
blocking (other than knowing the operations their code must have
performed on the pipe). This is unchanged from the present code in
CVS.
An existing problem with ap_put_os_file(), described above, was fixed.
> It is also very
>messy IMO.
I feel differently, of course :) It took around 10 lines of code to
implement the state in Unix, and it fixes problems in the current code
as well as with my original patch.
Only Unix code is included in this patch. Once we can agree on this
I'll revisit Win32 and OS/2. I need to regression test the current
Win32 code first anyway to understand what is broke and what is
working and what I think I can fix.
Index: src/lib/apr/file_io/unix/filedup.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/filedup.c,v
retrieving revision 1.22
diff -u -r1.22 filedup.c
--- src/lib/apr/file_io/unix/filedup.c 2000/06/18 02:38:51 1.22
+++ src/lib/apr/file_io/unix/filedup.c 2000/06/18 18:25:56
@@ -84,6 +84,7 @@
#endif
(*new_file)->buffer = ap_palloc(p, APR_FILE_BUFSIZE);
}
+ (*new_file)->blocking = old_file->blocking; /* this is the way dup() works */
ap_register_cleanup((*new_file)->cntxt, (void *)(*new_file), ap_unix_file_cleanup,
ap_null_cleanup);
return APR_SUCCESS;
Index: src/lib/apr/file_io/unix/fileio.h
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/fileio.h,v
retrieving revision 1.21
diff -u -r1.21 fileio.h
--- src/lib/apr/file_io/unix/fileio.h 2000/06/13 16:30:39 1.21
+++ src/lib/apr/file_io/unix/fileio.h 2000/06/18 18:25:56
@@ -117,6 +117,7 @@
int pipe;
ap_interval_time_t timeout;
int buffered;
+ enum {BLK_UNKNOWN, BLK_OFF, BLK_ON } blocking;
int ungetchar; /* Last char provided by an unget op. (-1 = no char)*/
/* Stuff for buffered mode */
Index: src/lib/apr/file_io/unix/open.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/open.c,v
retrieving revision 1.56
diff -u -r1.56 open.c
--- src/lib/apr/file_io/unix/open.c 2000/06/17 18:16:10 1.56
+++ src/lib/apr/file_io/unix/open.c 2000/06/18 18:25:56
@@ -109,6 +107,7 @@
(*new)->fname = ap_pstrdup(cont, fname);
+ (*new)->blocking = BLK_ON;
(*new)->buffered = (flag & APR_BUFFERED) > 0;
if ((*new)->buffered) {
@@ -213,6 +212,7 @@
}
(*file)->eof_hit = 0;
(*file)->buffered = 0;
+ (*file)->blocking = BLK_UNKNOWN; /* in case it is a pipe */
(*file)->timeout = -1;
(*file)->filedes = *dafile;
/* buffer already NULL;
@@ -246,6 +246,7 @@
(*thefile)->filedes = STDERR_FILENO;
(*thefile)->cntxt = cont;
(*thefile)->buffered = 0;
+ (*thefile)->blocking = BLK_UNKNOWN;
(*thefile)->fname = NULL;
(*thefile)->eof_hit = 0;
Index: src/lib/apr/file_io/unix/pipe.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/pipe.c,v
retrieving revision 1.34
diff -u -r1.34 pipe.c
--- src/lib/apr/file_io/unix/pipe.c 2000/06/17 17:04:16 1.34
+++ src/lib/apr/file_io/unix/pipe.c 2000/06/18 18:25:56
@@ -82,6 +82,8 @@
#endif /* BEOS_BONE */
#endif /* !BEOS */
+
+ thepipe->blocking = BLK_OFF;
return APR_SUCCESS;
}
@@ -89,12 +91,17 @@
{
if (thepipe->pipe == 1) {
thepipe->timeout = timeout;
+ if (thepipe->blocking != BLK_OFF) { /* blocking or unknown state */
+ return pipenonblock(thepipe);
+ }
return APR_SUCCESS;
}
return APR_EINVAL;
}
-ap_status_t ap_create_pipe(ap_file_t **in, ap_file_t **out, ap_pool_t *cont)
+ap_status_t ap_create_pipe(ap_file_t **in, int input_blocking,
+ ap_file_t **out, int output_blocking,
+ ap_pool_t *cont)
{
int filedes[2];
@@ -108,6 +115,7 @@
(*in)->pipe = 1;
(*in)->fname = ap_pstrdup(cont, "PIPE");
(*in)->buffered = 0;
+ (*in)->blocking = BLK_ON;
(*in)->timeout = -1;
(*in)->ungetchar = -1;
#if APR_HAS_THREADS
@@ -120,14 +128,20 @@
(*out)->pipe = 1;
(*out)->fname = ap_pstrdup(cont, "PIPE");
(*out)->buffered = 0;
+ (*out)->blocking = BLK_ON;
(*out)->timeout = -1;
#if APR_HAS_THREADS
(*in)->thlock = NULL;
#endif
- pipenonblock(*in);
- pipenonblock(*out);
+ if (!input_blocking) {
+ pipenonblock(*in);
+ }
+ if (!output_blocking) {
+ pipenonblock(*out);
+ }
+
return APR_SUCCESS;
}
@@ -171,6 +185,8 @@
#endif /* BEOS_BONE */
#endif /* !BEOS_R5 */
+
+ thepipe->blocking = BLK_ON;
return APR_SUCCESS;
}
Index: src/lib/apr/include/apr_file_io.h
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/include/apr_file_io.h,v
retrieving revision 1.55
diff -u -r1.55 apr_file_io.h
--- src/lib/apr/include/apr_file_io.h 2000/06/17 17:04:17 1.55
+++ src/lib/apr/include/apr_file_io.h 2000/06/18 18:25:57
@@ -541,17 +541,19 @@
/*
-=head1 ap_status_t ap_create_pipe(ap_file_t **in, ap_file_t **out, ap_pool_t *cont)
+=head1 ap_status_t ap_create_pipe(ap_file_t **in, int input_blocking, ap_file_t **out, int output_blocking, ap_pool_t *cont)
B<Create an anonymous pipe.>
arg 1) The file descriptor to use as input to the pipe.
- arg 2) The file descriptor to use as output from the pipe.
- arg 3) The pool to operate on.
+ arg 2) Flag: non-zero if you want the input end of the pipe returned in blocking mode
+ arg 3) The file descriptor to use as output from the pipe.
+ arg 4) Flag: non-zero if you want the output end of the pipe returned in blocking mode
+ arg 5) The pool to operate on.
=cut
*/
-ap_status_t ap_create_pipe(ap_file_t **in, ap_file_t **out, ap_pool_t *cont);
+ap_status_t ap_create_pipe(ap_file_t **in, int input_blocking, ap_file_t **out, int output_blocking, ap_pool_t *cont);
/*
Index: src/lib/apr/test/testpipe.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/test/testpipe.c,v
retrieving revision 1.7
diff -u -r1.7 testpipe.c
--- src/lib/apr/test/testpipe.c 2000/04/15 02:56:50 1.7
+++ src/lib/apr/test/testpipe.c 2000/06/18 18:26:01
@@ -86,7 +86,7 @@
fprintf(stdout, "Testing pipe functions.\n");
fprintf(stdout, "\tCreating pipes.......");
- if (ap_create_pipe(&readp, &writep, context) != APR_SUCCESS) {
+ if (ap_create_pipe(&readp, 1, &writep, 1, context) != APR_SUCCESS) {
perror("Didn't create the pipe");
exit(-1);
}
Index: src/lib/apr/threadproc/unix/proc.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/threadproc/unix/proc.c,v
retrieving revision 1.30
diff -u -r1.30 proc.c
--- src/lib/apr/threadproc/unix/proc.c 2000/06/06 21:45:11 1.30
+++ src/lib/apr/threadproc/unix/proc.c 2000/06/18 18:26:03
@@ -71,50 +71,36 @@
ap_int32_t out, ap_int32_t err)
{
ap_status_t status;
+ int child_blocking, parent_blocking;
+
if (in != 0) {
- if ((status = ap_create_pipe(&attr->child_in, &attr->parent_in,
- attr->cntxt)) != APR_SUCCESS) {
+ child_blocking = in == APR_FULL_BLOCK || in == APR_CHILD_BLOCK;
+ parent_blocking = in == APR_FULL_BLOCK || in == APR_PARENT_BLOCK;
+
+ if ((status = ap_create_pipe(&attr->child_in, child_blocking,
+ &attr->parent_in, parent_blocking,
+ attr->cntxt)) != APR_SUCCESS) {
return status;
}
- switch (in) {
- case APR_FULL_BLOCK:
- ap_block_pipe(attr->child_in);
- ap_block_pipe(attr->parent_in);
- case APR_PARENT_BLOCK:
- ap_block_pipe(attr->parent_in);
- case APR_CHILD_BLOCK:
- ap_block_pipe(attr->child_in);
- }
}
if (out) {
- if ((status = ap_create_pipe(&attr->parent_out, &attr->child_out,
- attr->cntxt)) != APR_SUCCESS) {
+ child_blocking = out == APR_FULL_BLOCK || out == APR_CHILD_BLOCK;
+ parent_blocking = out == APR_FULL_BLOCK || out == APR_PARENT_BLOCK;
+
+ if ((status = ap_create_pipe(&attr->parent_out, parent_blocking,
+ &attr->child_out, child_blocking,
+ attr->cntxt)) != APR_SUCCESS) {
return status;
}
- switch (out) {
- case APR_FULL_BLOCK:
- ap_block_pipe(attr->child_out);
- ap_block_pipe(attr->parent_out);
- case APR_PARENT_BLOCK:
- ap_block_pipe(attr->parent_out);
- case APR_CHILD_BLOCK:
- ap_block_pipe(attr->child_out);
- }
}
if (err) {
- if ((status = ap_create_pipe(&attr->parent_err, &attr->child_err,
- attr->cntxt)) != APR_SUCCESS) {
+ child_blocking = err == APR_FULL_BLOCK || err == APR_CHILD_BLOCK;
+ parent_blocking = err == APR_FULL_BLOCK || err == APR_PARENT_BLOCK;
+ if ((status = ap_create_pipe(&attr->parent_err, parent_blocking,
+ &attr->child_err, child_blocking,
+ attr->cntxt)) != APR_SUCCESS) {
return status;
}
- switch (err) {
- case APR_FULL_BLOCK:
- ap_block_pipe(attr->child_err);
- ap_block_pipe(attr->parent_err);
- case APR_PARENT_BLOCK:
- ap_block_pipe(attr->parent_err);
- case APR_CHILD_BLOCK:
- ap_block_pipe(attr->child_err);
- }
}
return APR_SUCCESS;
}
@@ -124,7 +110,7 @@
ap_file_t *parent_in)
{
if (attr->child_in == NULL && attr->parent_in == NULL)
- ap_create_pipe(&attr->child_in, &attr->parent_in, attr->cntxt);
+ ap_create_pipe(&attr->child_in, 0, &attr->parent_in, 0, attr->cntxt);
if (child_in != NULL)
ap_dupfile(&attr->child_in, child_in, attr->cntxt);
@@ -140,7 +126,7 @@
ap_file_t *parent_out)
{
if (attr->child_out == NULL && attr->parent_out == NULL)
- ap_create_pipe(&attr->child_out, &attr->parent_out, attr->cntxt);
+ ap_create_pipe(&attr->child_out, 0, &attr->parent_out, 0, attr->cntxt);
if (child_out != NULL)
ap_dupfile(&attr->child_out, child_out, attr->cntxt);
@@ -156,7 +142,7 @@
ap_file_t *parent_err)
{
if (attr->child_err == NULL && attr->parent_err == NULL)
- ap_create_pipe(&attr->child_err, &attr->parent_err, attr->cntxt);
+ ap_create_pipe(&attr->child_err, 0, &attr->parent_err, 0, attr->cntxt);
if (child_err != NULL)
ap_dupfile(&attr->child_err, child_err, attr->cntxt);
Index: src/main/http_log.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/http_log.c,v
retrieving revision 1.56
diff -u -r1.56 http_log.c
--- src/main/http_log.c 2000/06/12 23:02:49 1.56
+++ src/main/http_log.c 2000/06/18 18:26:05
@@ -704,13 +704,10 @@
pl->p = p;
pl->program = ap_pstrdup(p, program);
pl->pid = NULL;
- if (ap_create_pipe(&ap_piped_log_read_fd(pl), &ap_piped_log_write_fd(pl), p) != APR_SUCCESS) {
- int save_errno = errno;
- errno = save_errno;
+ if (ap_create_pipe(&ap_piped_log_read_fd(pl), 1,
+ &ap_piped_log_write_fd(pl), 1, p) != APR_SUCCESS) {
return NULL;
}
- ap_block_pipe(ap_piped_log_read_fd(pl));
- ap_block_pipe(ap_piped_log_write_fd(pl));
ap_register_cleanup(p, pl, piped_log_cleanup, piped_log_cleanup_for_exec);
if (piped_log_spawn(pl) == -1) {
int save_errno = errno;
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
> From: rbb@covalent.net
> Date: Sat, 17 Jun 2000 06:05:36 -0700 (PDT)
>
> I remember the problem now. Try setting a timeout on a pipe that wasn't
> setup blocking.
Thanks for the warning! When I changed testpipe.c to handle the new
parms, I just put 0 there to have it behave as before. Now I see the
brokenness.
> Remember, that pipes allow being timed out. I REALLY
> think that having some pipes that can be timed out and some that can't is
> a serious mistake.
We just need to make sure that if/when a timeout is set that we ensure
that the pipe is non-blocking, since our implementation of timeouts
requires that it is non-blocking before it enters the read/write code
(your solution 2 below).
> There are two solutions to this. 1) Do what we did
> and always set a pipe up for non-blocking and let the read/write code do
> the blocking stuff. 2) Keep a state flag with the pipe to determine if
> it has been set non-blocking. The second method has the unfortunate
> problem of not really working with ap_get_os_file well. It is also very
> messy IMO.
Lots of extra syscalls are much more messy IMHO. The current
assumption of non-blocking-state madeby the code that sets timeouts is
messy IMHO.
I'll put together another patch that handles timeouts, and I'll look
into the ap_get_os_file() issue too to try to understand what you are
alluding to.
>
> I am -0.5, but I need to review the code in much more detail, which I'm
> not doing right now. I will review it later this weekend or
> Monday. Please hold off committing until then.
>
> Ryan
>
You've convinced me that the current patch is broken (the timeout
issue), so don't worry about me committing it yet :) I have no doubt
that any remaining issues can be fixed, that the interface for the app
won't be more clumsy, and that the reduced syscalls will justify the
several extra lines of code here and there within APR.
Thanks and best wishes,
Jeff
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
> > pipes were non-blocking for processes, and that didn't work. Then they
> > were created blocking, and that didn't work. Finally, we had to allow
> > processes to specify which sides of the pipes were supposed to block and
> > which weren't.
>
> I guess you're referring to my comment above about both ends being the
> same. This has no effect on the process creator which wants to
> specify both sides of the pipe. I wasn't taking that away, but I was
> affecting how much work has to be done by ap_setprocattr_io() to meet
> the requirements of its caller.
>
> Just look at the patch I posted later. Most of the calls to
> ap_create_pipe() are hidden inside ap_setprocattr_io() anyway.
I remember the problem now. Try setting a timeout on a pipe that wasn't
setup blocking. Remember, that pipes allow being timed out. I REALLY
think that having some pipes that can be timed out and some that can't is
a serious mistake. There are two solutions to this. 1) Do what we did
and always set a pipe up for non-blocking and let the read/write code do
the blocking stuff. 2) Keep a state flag with the pipe to determine if
it has been set non-blocking. The second method has the unfortunate
problem of not really working with ap_get_os_file well. It is also very
messy IMO.
I am -0.5, but I need to review the code in much more detail, which I'm
not doing right now. I will review it later this weekend or
Monday. Please hold off committing until then.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
> From: rbb@covalent.net
> Date: Fri, 16 Jun 2000 19:54:12 -0700 (PDT)
>
> > or b) Add a flag to ap_create_pipe() with values APR_PIPE_BLOCKING or
> > APR_PIPE_NONBLOCKING. (I don't think we need different values
> > for each end of the pipe.) Do the right thing inside. This helps
> > OS/2 in particular since the pipe can be created as desired with
> > no extra syscall.
>
> If you are going to block the pipe, you need to specify the sides of the
> pipe. This also is a lesson learned from experience in APR.
Yea, as I started coding this became pretty obvious. Look at the
patch I posted later.
Also, note that the reduction in syscalls was not simply due to fixing
the missing break statements, as we jumped to the last case often
enough. It is simply that we want blocking behavior for more ends of
our pipes than we want non-blocking.
> pipes were non-blocking for processes, and that didn't work. Then they
> were created blocking, and that didn't work. Finally, we had to allow
> processes to specify which sides of the pipes were supposed to block and
> which weren't.
I guess you're referring to my comment above about both ends being the
same. This has no effect on the process creator which wants to
specify both sides of the pipe. I wasn't taking that away, but I was
affecting how much work has to be done by ap_setprocattr_io() to meet
the requirements of its caller.
Just look at the patch I posted later. Most of the calls to
ap_create_pipe() are hidden inside ap_setprocattr_io() anyway.
Have fun...
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
>
> a) Let pipes default to blocking. Add new APR function
> ap_nonblock_pipe() for callers that want them non-blocking.
That was in the original design. I can't remember why, but it doesn't
work. I'll need to dig to figure it out.
> or b) Add a flag to ap_create_pipe() with values APR_PIPE_BLOCKING or
> APR_PIPE_NONBLOCKING. (I don't think we need different values
> for each end of the pipe.) Do the right thing inside. This helps
> OS/2 in particular since the pipe can be created as desired with
> no extra syscall.
If you are going to block the pipe, you need to specify the sides of the
pipe. This also is a lesson learned from experience in APR. Originally,
pipes were non-blocking for processes, and that didn't work. Then they
were created blocking, and that didn't work. Finally, we had to allow
processes to specify which sides of the pipes were supposed to block and
which weren't. I can't even think about recalling why right now. And I'm
not looking at code at all tonight. I'll look at all of this sometime
later this weekend or on Monday. For right now, I am -1 for changing the
pipes logic, because it was changed to what it is throught hard won
experience.
> I prefer b). It will be clearer what is happening when you see a call
> to ap_create_pipe().
>
> ap_nonblock_pipe() may be needed anyway, but I'd like to avoid it for
> now.
>
> I don't know how much this will speed up mod_cgi vs. mod_cgid...
> Probably not much, since fcntl should be pretty cheap. It will be
> cleaner and traces of mod_cgi won't be quite so disturbing.
>
> Any comments? Any preferences for a) vs. b) vs. your ideas?
See above.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------