httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] final brigade buffering
Date Thu, 08 Feb 2001 20:04:28 GMT
On Thu, Feb 08, 2001 at 06:52:01AM -0800, rbb@covalent.net wrote:
>...
> One more thing about this.  How we flush is not a feature of where we are,
> it is a feature of a brigade.  Apache happens to use the same brigade in
> a multitidude of locations throughout the code.  When I write my word
> processor using brigades, my flush function and context will always be the
> same, when the brigade gets to full, I will be flushing to disk, into a
> specific file.
> 
> This also fits very well into the brigades that are stored in a filter's
> ctx field.  Those brigades don't move, they stay in that f->ctx block, and
> thus the flush mechanism is different for them than for brigades that we
> pass down the stack, but it is always the same for that brigade.
> 
> Brigades are data stores, and a data store doesn't care where they are
> when they flush data, they just care that they have to flush, and they
> flush correctly.

Hrm. I'm sorry that I wasn't clear enough... it is pretty clear to me about
the need, so I obviously missed a point in explaining it well enough. Let's
use code this time. That is quite a bit more rigorous than english :-)

Consider this handler and filter (and that the filter is on the chain):

void my_handler(...)
{
    bb = brigade_create()
    bb->flush = my_flush;
    bb->flush_ctx = r->output_filters;
    
    apr_brigade_write(bb, "some big-ass string that causes a flush");
    apr_brigade_write(bb, "another big-ass string");
}

apr_status_t my_flush(bb, ctx)
{
    return ap_pass_brigade(ctx, bb);
}

apr_status_t some_filter(f, bb)
{
    bb->flush = some_filter_flush;
    bb->flush_ctx = f->next;
    
    apr_brigade_write(bb, "small or big, this doesn't matter");
}

apr_status_t some_filter_flush(bb, ctx)
{
    return ap_pass_brigade(ctx, bb);
}


The above code is subtly broken because of the flush function and context
are placed into the brigade. The values are very much context-dependent. By
placing them into the brigade itself, it is creating a situation very
similar to globals: you have contention for the values that should be in
there. Each caller of apr_brigade_write needs a different set, because they
have different needs on *how* to get the brigade flushed.

Since each caller has their own needs, it makes the most sense to place them
directly into the call itself (as parameters). Otherwise, every single call
will be preceded by setting flush_func/flush_ctx. In that case, it will be
immediately apparent that they should be paremeters :-)

I'd suggest that all the brigade write functions have a standardized
prototype along the lines of:

  apr_status_t apr_brigade_whatever(bb, flush_func, flush_ctx, ...params)

flush_func can be NULL; if so, then the brigade buffering may need to copy
the values into a heap bucket (regardless of size).


For those that haven't seen the bug in the above code yet, consider the
*second* call to apr_brigade_write in the handler. Because the first call
caused a flush through the filter stack, and some_filter wrote over the
flush values, then the second write in the handler gets flushed to the wrong
location.

Cheers,
-g

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

Mime
View raw message