httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: [PATCH] final brigade buffering
Date Fri, 09 Feb 2001 01:06:33 GMT

You were perfectly clear, but you are mixing APIs.  There are two Apache
API's and one APR-util API.

The two Apache API's are:

	ap_r*
	ap_f*

The one APR-util API is:

	apr_brigade_*

It is perfectly possible for module writers to call any of the three at
any time, but they have to know what they are doing.

Our ap_f* calls setup the ctx correctly for when they call apr_brigade_*,
so any module author using the Apache API doesn't have to worry about it
at all.  The APR-util API expect's the programmer to think about what they
want to do, and write the code accordingly.

The problem with the code below, is that you are looking to use the APR
functions without really thinking about how they work.  Our brigades are
used for filters, so how we flush depends on which filter we are in.  In
the grand scheme of things, brigades are data stores, and how you flush
depends on the properties of the brigade, not on where you are.

Yes, if you write code like what you have below, you will not get what you
expect.  But, you are using the wrong tool for the job.  For what you
want, you don't want to use the brigade functions, you want to use the
Apache specific functions.

Ryan


> 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/
> 
> 


_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------




Mime
View raw message