mod_asis handler bug

[Bill Stoddard <bi...@wstoddard.com>]

In the mod_asis handler, we call:

apr_file_open()
ap_send_fd()
apr_file_close()

The problem is that the brigade created by ap_send_fd is not be flushed to the network
before the apr_file_close (because we've not hit the min bytes to write threshold and one
of the filters is buffering).  I see a couple of solutions:

1. Not call apr_file_close() if we call ap_send_fd().  The bucket cleanup code will handle
closing the file.

2. Chase the call to ap_send_fd() with a call to ap_rflush() and call apr_file_close as is
happening now.

3. Variation on 2)... Pass an additional option to ap_send_fd to specify whether to flush
or not. And call apr_file_close() as is happening now.

I am leaning toward 3 for this specific case.  This way, the handler can tell ap_send_fd
to flush and be assured the bytes have been sent to the network before it returns.

Thoughts?

Bill

Re: mod_asis handler bug

["Roy T. Fielding" <fi...@ebuilt.com>]

On Fri, Jul 27, 2001 at 04:14:00PM -0400, Bill Stoddard wrote:
> There another bug lurking in mod_asis (reported by Ken Bruinsma in IBM).
> We are creating the file bucket with file offset of 0.  Problem is that
> we have already read in part of the file (the headers) a bit earlier.
> Need to give some thought to the best way to fix this (and what all the
> implications are).

The offset in a normal file bucket should be relative to the current read
pointer.  We need to differentiate between shared file buckets (with absolute
offsets) versus unshared file buckets.  Then we can get rid of the extra
fseek call as well.

....Roy

Re: mod_asis handler bug

[Cliff Woolley <cl...@yahoo.com>]

On Fri, 27 Jul 2001, Bill Stoddard wrote:

> > There another bug lurking in mod_asis (reported by Ken Bruinsma in
> > IBM). We are creating the file bucket with file offset of 0.  Problem
> > is that we have already read in part of the file (the headers) a bit
> > earlier. Need to give some thought to the best way to fix this (and
> > what all the implications are).
> >
> > Perhaps pass in a -1 for offset on the apr_bucket_file_create() when
> > we want to rely on the system file pointer?  Which reminds me that I
> > still need to look at the apr_file_seek issue Cliff, Ryan and I were
> > discussing a few weeks past.
> >
> > Cliff, I am about out of time today so if you want to pursue
> > this, be my guest:-)
>
> But if the file is sent by sendfile, the system filepointer is
> useless.  We -must- pass the correct offset into
> apr_bucket_file_create().

Hoooo... that's fugly.  There are a few possible solutions.

(1) Keep track of the number of bytes read in from the file in
ap_scan_script_header_err() and use that as the offset when calling
apr_bucket_file_create().

(2) Have one of these "unshared file bucket" thingies as Roy suggests and
as Ryan, Bill, and I discussed a while back which relies on the system's
file pointer.  The only problem with that, as I mentioned when we last had
this discussiond, is that there's very little you can do to such a bucket
before it becomes a "shared" file bucket.  If you copy, split, etc, you
all of the sudden have to have absolute offsets and seek every time.
That could get tricky to handle.  And it would cause the kind of thing
that OtherBill initially implemented the other day when fixing up the
APR_HAS_LARGE_FILES stuff, which is to have a single apr_file_t and put it
in a whole bunch of file buckets, each with a different offset; right now,
such behavior works but is just sub-optimal performance-wise.  With
"unshared" file buckets, all of the sudden you'd have a bunch of unshared
file buckets that pointed to a single apr_file_t that really WAS shared.
Refcounting the apr_file_t causes problems as well, particularly when your
apr_file_t is APR_XTHREAD.

I kinda think we need to stick with #1.  It's way easier and avoids all
these wacky edge cases that could cause #2 to exhibit unpredictable
behavior.

--Cliff



--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA

Re: mod_asis handler bug

[Bill Stoddard <bi...@wstoddard.com>]

But if the file is sent by sendfile, the system filepointer is useless. We -must- pass the
correct offset into apr_bucket_file_create().

Bill

> There another bug lurking in mod_asis (reported by Ken Bruinsma in IBM). We are creating
> the file bucket with file offset of 0.  Problem is that we have already read in part of
> the file (the headers) a bit earlier. Need to give some thought to the best way to fix
> this (and what all the implications are).
>
> Perhaps pass in a -1 for offset on the apr_bucket_file_create() when we want to rely on
> the system file pointer? Which reminds me that I still need to look at the apr_file_seek
> issue Cliff, Ryan and I were discussing a few weeks past.
>
> Cliff, I am about out of time today so if you want to pursue this, be my guest:-)
>
> Bill
>
>
> ----- Original Message -----
> From: "Cliff Woolley" <cl...@yahoo.com>
> To: <ne...@apache.org>
> Sent: Friday, July 27, 2001 3:40 PM
> Subject: Re: mod_asis handler bug
>
>
> > On Fri, 27 Jul 2001, Bill Stoddard wrote:
> >
> > > What Ryan is suggesting is slightly different than what either of us
> > > has proposed.
> >
> > Yeah... that's because I never proposed it.  But I _had_ been thinking it
> > needed to be done.  :)
> >
> > I did a recursive grep a few days ago for ap_send_fd... it's currently
> > only being used by mod_asis, which is good.  Apache itself should just
> > about never use it, since we almost always have more work to do (eg
> > flushing the brigade, sending other stuff down the chain, etc), which
> > makes it more efficient if we build the brigade on our own.  ap_send_fd()
> > is the quick-and-dirty, old-api-compatible way to do it.
> >
> > >  His suggestion sends down the eos, not a flush.  Will
> > > commit this in a few.
> >
> > Have at it!  =-)
> >
> > --Cliff
> >
> > --------------------------------------------------------------
> >    Cliff Woolley
> >    cliffwoolley@yahoo.com
> >    Charlottesville, VA
> >
> >
>

Re: mod_asis handler bug

[Bill Stoddard <bi...@wstoddard.com>]

There another bug lurking in mod_asis (reported by Ken Bruinsma in IBM). We are creating
the file bucket with file offset of 0.  Problem is that we have already read in part of
the file (the headers) a bit earlier. Need to give some thought to the best way to fix
this (and what all the implications are).

Perhaps pass in a -1 for offset on the apr_bucket_file_create() when we want to rely on
the system file pointer? Which reminds me that I still need to look at the apr_file_seek
issue Cliff, Ryan and I were discussing a few weeks past.

Cliff, I am about out of time today so if you want to pursue this, be my guest:-)

Bill


----- Original Message -----
From: "Cliff Woolley" <cl...@yahoo.com>
To: <ne...@apache.org>
Sent: Friday, July 27, 2001 3:40 PM
Subject: Re: mod_asis handler bug


> On Fri, 27 Jul 2001, Bill Stoddard wrote:
>
> > What Ryan is suggesting is slightly different than what either of us
> > has proposed.
>
> Yeah... that's because I never proposed it.  But I _had_ been thinking it
> needed to be done.  :)
>
> I did a recursive grep a few days ago for ap_send_fd... it's currently
> only being used by mod_asis, which is good.  Apache itself should just
> about never use it, since we almost always have more work to do (eg
> flushing the brigade, sending other stuff down the chain, etc), which
> makes it more efficient if we build the brigade on our own.  ap_send_fd()
> is the quick-and-dirty, old-api-compatible way to do it.
>
> >  His suggestion sends down the eos, not a flush.  Will
> > commit this in a few.
>
> Have at it!  =-)
>
> --Cliff
>
> --------------------------------------------------------------
>    Cliff Woolley
>    cliffwoolley@yahoo.com
>    Charlottesville, VA
>
>

Re: mod_asis handler bug

[Cliff Woolley <cl...@yahoo.com>]

On Fri, 27 Jul 2001, Bill Stoddard wrote:

> What Ryan is suggesting is slightly different than what either of us
> has proposed.

Yeah... that's because I never proposed it.  But I _had_ been thinking it
needed to be done.  :)

I did a recursive grep a few days ago for ap_send_fd... it's currently
only being used by mod_asis, which is good.  Apache itself should just
about never use it, since we almost always have more work to do (eg
flushing the brigade, sending other stuff down the chain, etc), which
makes it more efficient if we build the brigade on our own.  ap_send_fd()
is the quick-and-dirty, old-api-compatible way to do it.

>  His suggestion sends down the eos, not a flush.  Will
> commit this in a few.

Have at it!  =-)

--Cliff

--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA

Re: mod_asis handler bug

[Bill Stoddard <bi...@wstoddard.com>]

What Ryan is suggesting is slightly different than what either of us has proposed.  His
suggestion sends down the eos, not a flush.  Will commit this in a few.

Bill

> On Fri, 27 Jul 2001, Ryan Bloom wrote:
>
> >
> > Why not just re-write it to make more sense?
> >
> > apr_file_open()
> > apr_bucket_file_create()
> > apr_brigade_create()
> > APR_BUCKET_INSERT_HEAD()
> > apr_bucket_eos_create()
> > APR_BUCKET_INSERT_TAIL()
> > ap_pass_brigade()
> > return OK
>
> I was going to do that anyway.  But to get what's there working with the
> fewest number of lines changed, all that has to happen is to - the
> apr_file_close() lines.
>
> I didn't make the change to the above because I wasn't sure what the
> behavior would be within the 'asis' concept and whether it would work as
> expected with filters in the chain and all.  But then again all that
> ap_send_fd() is doing under the covers is the above anyway, so it doesn't
> really make a difference.
>
>
> --------------------------------------------------------------
>    Cliff Woolley
>    cliffwoolley@yahoo.com
>    Charlottesville, VA
>
>

Re: mod_asis handler bug

[Cliff Woolley <cl...@yahoo.com>]

On Fri, 27 Jul 2001, Ryan Bloom wrote:

>
> Why not just re-write it to make more sense?
>
> apr_file_open()
> apr_bucket_file_create()
> apr_brigade_create()
> APR_BUCKET_INSERT_HEAD()
> apr_bucket_eos_create()
> APR_BUCKET_INSERT_TAIL()
> ap_pass_brigade()
> return OK

I was going to do that anyway.  But to get what's there working with the
fewest number of lines changed, all that has to happen is to - the
apr_file_close() lines.

I didn't make the change to the above because I wasn't sure what the
behavior would be within the 'asis' concept and whether it would work as
expected with filters in the chain and all.  But then again all that
ap_send_fd() is doing under the covers is the above anyway, so it doesn't
really make a difference.


--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA

Re: mod_asis handler bug

[Ryan Bloom <rb...@covalent.net>]

Why not just re-write it to make more sense?

apr_file_open()
apr_bucket_file_create()
apr_brigade_create()
APR_BUCKET_INSERT_HEAD()
apr_bucket_eos_create()
APR_BUCKET_INSERT_TAIL()
ap_pass_brigade()
return OK

Ryan

On Friday 27 July 2001 08:57, Bill Stoddard wrote:
> In the mod_asis handler, we call:
>
> apr_file_open()
> ap_send_fd()
> apr_file_close()
>
> The problem is that the brigade created by ap_send_fd is not be flushed to
> the network before the apr_file_close (because we've not hit the min bytes
> to write threshold and one of the filters is buffering).  I see a couple of
> solutions:
>
> 1. Not call apr_file_close() if we call ap_send_fd().  The bucket cleanup
> code will handle closing the file.
>
> 2. Chase the call to ap_send_fd() with a call to ap_rflush() and call
> apr_file_close as is happening now.
>
> 3. Variation on 2)... Pass an additional option to ap_send_fd to specify
> whether to flush or not. And call apr_file_close() as is happening now.
>
> I am leaning toward 3 for this specific case.  This way, the handler can
> tell ap_send_fd to flush and be assured the bytes have been sent to the
> network before it returns.
>
> Thoughts?
>
> Bill

-- 

_____________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
Covalent Technologies			rbb@covalent.net
-----------------------------------------------------------------------------

Re: mod_asis handler bug

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

From: "Bill Stoddard" <bi...@wstoddard.com>
Sent: Friday, July 27, 2001 10:57 AM


> In the mod_asis handler, we call:
> 
> apr_file_open()
> ap_send_fd()
> apr_file_close()
> 
> The problem is that the brigade created by ap_send_fd is not be flushed to the network
> before the apr_file_close (because we've not hit the min bytes to write threshold and one
> of the filters is buffering).  I see a couple of solutions:
> 
> 1. Not call apr_file_close() if we call ap_send_fd().  The bucket cleanup code will handle
> closing the file.
> 
> 2. Chase the call to ap_send_fd() with a call to ap_rflush() and call apr_file_close as is
> happening now.
> 
> 3. Variation on 2)... Pass an additional option to ap_send_fd to specify whether to flush
> or not. And call apr_file_close() as is happening now.
> 
> I am leaning toward 3 for this specific case.  This way, the handler can tell ap_send_fd
> to flush and be assured the bytes have been sent to the network before it returns.
> 
> Thoughts?

None of the above?  Have ap_send_fd play the same game as a bucket, duping and refcounting
the file to a safe bucket?

Re: mod_asis handler bug

[Cliff Woolley <cl...@yahoo.com>]

On Fri, 27 Jul 2001, Bill Stoddard wrote:

> In the mod_asis handler, we call:
>
> apr_file_open()
> ap_send_fd()
> apr_file_close()

I noticed that.  I actually spent a little time tracking down the
apr_file_close() calls the other day because I wasn't sure why they are
in there, and it turns out that (give or take about five intervening
function renames) they've been around ever since mod_asis was using
fopen()/fclose() directly.  The apr_file_close() calls should go away
since the pool cleanup does the work for us already and does it at "the
right time."

> 1. Not call apr_file_close() if we call ap_send_fd().

Yes.

> 2. Chase the call to ap_send_fd() with a call to ap_rflush() and call
> apr_file_close as is happening now.
>
> 3. Variation on 2)... Pass an additional option to ap_send_fd to
> specify whether to flush or not. And call apr_file_close() as is
> happening now.

#3 is better for performance than #2 because it spares us the creation of
a second brigade and an extra trip down the filter stack.  +1 to that in
addition to number 1.

--Cliff


--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA