OLD_WRITE stuff (was: Re: 2.0.34 - erratic behavior with autoindexes)

[Greg Stein <gs...@lyra.org>]

On Tue, Apr 02, 2002 at 01:50:59PM -0800, Ryan Bloom wrote:
>...
> > chain.  Bill S. stuck his head in here and said something about a rule
> > that if old_write is ever used, it has to always be used.
> 
> If you read through mod_include, you will see that whenever we create a
> sub-request, we send the output from the current filter to the next
> filter.  This is required to make sure that things are output in the
> correct order.  If you are a handler and are generating data through
> ap_r* functions, you must flush the filter stack so that you are sure
> that OLD_WRITE isn't buffering for you.

That's not true. OLD_WRITE was designed to allow a mix/match of writing to
the stack *and* using the ap_r* functions.

The design is that ap_r* implies a write into the filter stack. That write
is optimized to use a brigade for buffering (so every ap_rputc() doesn't
traverse the filter stack). If OLD_WRITE isn't at the top, then it doesn't
do the buffering (because there are filters between the top and the
OLD_WRITE buffers).

If somebody writes directly into the filter stack, then it prepends any
buffered content to whatever is written, and then passes it all down the
stack.

[ just reviewed the code; it seems to still match the original design ]

It all looks pretty good, but there might be a problem where:

*) ap_r* is used, so OLD_WRITE is inserted, and the content is stored into
   its buffer.

*) another filter is inserted "above" OLD_WRITE, so it is not the top. ap_r*
   content should now go into this new filter.

*) ap_r* is called again, but it thinks it cannot buffer. it passes the new
   data directly into the filter stack.
   
   BUG: the previously-buffered content is not prepended.


So... the bug might appear because of the increased dynamicity of the filter
chain nowadays. Back when OLD_WRITE was written, it was perceived that the
chain would be quite static by the time the first write occurred.

Note that OLD_WRITE tries quite hard to keep its filter on "top" (its filter
type is RESOURCE-10). If something broke that, then it is possible to hit
the above bug.

The fix to the (potential) bug is to change line 1370 in server/protocol.c
to concat the brigades, similar to ap_old_write_filter() (defined just
above, at line 1316).

Another question is whether anybody defines their filter type as less than
RESOURCE-10 and ends up ahead of OLD_WRITE.

And lastly: I just realized that if a filter gets in front of OLD_WRITE,
then the branch at line 1366 will get called for each ap_r* call. If that is
a series of 1000's of ap_rputc() calls, then you'll end up creating
thousands of brigades for those transient buckets(!).

[ a similar brigade over-creation can occur if you alternate ap_r* and
  writing to the filter stack; ap_old_write_filter() will forget about the
  brigade it created, so the next ap_r* needs to recreate one ]

The answer might be to test for brigade-empty rather than ctx->bb == NULL.
Then the branch at 1366 can store and reuse a brigade in ctx->bb.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Re: OLD_WRITE stuff (was: Re: 2.0.34 - erratic behavior with autoindexes)

[Greg Stein <gs...@lyra.org>]

On Tue, Apr 02, 2002 at 02:56:33PM -0800, Ryan Bloom wrote:
> 
> Everything below is true, assuming you are not talking about
> sub-requests.  According to the stack that Greg posted, the OLD_WRITE
> filter was not found in the sub-request filter stack.  That is to be
> expected, because the OLD_WRITE filter is a RESOURCE filter.  Since
> RESOURCE filters are not kept for sub-requests, the data must be
> flushed.

Right.

> Hmmmmm....  Actually it seems like Resource filters should be kept for
> sub-requests that are created with a filter list.  If that is the case,
> then it looks like the OLD_WRITE filter should be in the filter list.

If the sub-request goes, then its resource filters should go. Those filters
are associated with that particular sub-request. That implies that the
resource filters should be flushed before they die.

[ but we should probably have a SUBREQ_ENDING_FLUSH bucket so the SUBREQ
  filter can chew it up; we don't want to flush all the way to the network
  just cuz the subreq is disappearing. ]

> Unless, the OLD_WRITE filter is being added after the sub-request is
> created......

The filter is added at the first call to ap_r*(). We decided on the lazy
approach so that pure-brigade systems would not suffer the overhead.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

RE: OLD_WRITE stuff (was: Re: 2.0.34 - erratic behavior with autoindexes)

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

Everything below is true, assuming you are not talking about
sub-requests.  According to the stack that Greg posted, the OLD_WRITE
filter was not found in the sub-request filter stack.  That is to be
expected, because the OLD_WRITE filter is a RESOURCE filter.  Since
RESOURCE filters are not kept for sub-requests, the data must be
flushed.

Hmmmmm....  Actually it seems like Resource filters should be kept for
sub-requests that are created with a filter list.  If that is the case,
then it looks like the OLD_WRITE filter should be in the filter list.

Unless, the OLD_WRITE filter is being added after the sub-request is
created......

Lots to look at......

Ryan

> On Tue, Apr 02, 2002 at 01:50:59PM -0800, Ryan Bloom wrote:
> >...
> > > chain.  Bill S. stuck his head in here and said something about a
rule
> > > that if old_write is ever used, it has to always be used.
> >
> > If you read through mod_include, you will see that whenever we
create a
> > sub-request, we send the output from the current filter to the next
> > filter.  This is required to make sure that things are output in the
> > correct order.  If you are a handler and are generating data through
> > ap_r* functions, you must flush the filter stack so that you are
sure
> > that OLD_WRITE isn't buffering for you.
> 
> That's not true. OLD_WRITE was designed to allow a mix/match of
writing to
> the stack *and* using the ap_r* functions.
> 
> The design is that ap_r* implies a write into the filter stack. That
write
> is optimized to use a brigade for buffering (so every ap_rputc()
doesn't
> traverse the filter stack). If OLD_WRITE isn't at the top, then it
doesn't
> do the buffering (because there are filters between the top and the
> OLD_WRITE buffers).
> 
> If somebody writes directly into the filter stack, then it prepends
any
> buffered content to whatever is written, and then passes it all down
the
> stack.
> 
> [ just reviewed the code; it seems to still match the original design
]
> 
> It all looks pretty good, but there might be a problem where:
> 
> *) ap_r* is used, so OLD_WRITE is inserted, and the content is stored
into
>    its buffer.
> 
> *) another filter is inserted "above" OLD_WRITE, so it is not the top.
> ap_r*
>    content should now go into this new filter.
> 
> *) ap_r* is called again, but it thinks it cannot buffer. it passes
the
> new
>    data directly into the filter stack.
> 
>    BUG: the previously-buffered content is not prepended.
> 
> 
> So... the bug might appear because of the increased dynamicity of the
> filter
> chain nowadays. Back when OLD_WRITE was written, it was perceived that
the
> chain would be quite static by the time the first write occurred.
> 
> Note that OLD_WRITE tries quite hard to keep its filter on "top" (its
> filter
> type is RESOURCE-10). If something broke that, then it is possible to
hit
> the above bug.
> 
> The fix to the (potential) bug is to change line 1370 in
server/protocol.c
> to concat the brigades, similar to ap_old_write_filter() (defined just
> above, at line 1316).
> 
> Another question is whether anybody defines their filter type as less
than
> RESOURCE-10 and ends up ahead of OLD_WRITE.
> 
> And lastly: I just realized that if a filter gets in front of
OLD_WRITE,
> then the branch at line 1366 will get called for each ap_r* call. If
that
> is
> a series of 1000's of ap_rputc() calls, then you'll end up creating
> thousands of brigades for those transient buckets(!).
> 
> [ a similar brigade over-creation can occur if you alternate ap_r* and
>   writing to the filter stack; ap_old_write_filter() will forget about
the
>   brigade it created, so the next ap_r* needs to recreate one ]
> 
> The answer might be to test for brigade-empty rather than ctx->bb ==
NULL.
> Then the branch at 1366 can store and reuse a brigade in ctx->bb.
> 
> Cheers,
> -g