httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: C-L filter problems (was: Re: cvs commit: ...)
Date Tue, 21 Nov 2000 22:08:18 GMT
On Tue, 21 Nov 2000, Greg Stein wrote:

> On Tue, Nov 21, 2000 at 08:17:22PM -0000, rbb@locus.apache.org wrote:
> >...
> >   --- http_core.h	2000/11/08 11:42:21	1.31
> >   +++ http_core.h	2000/11/21 20:17:18	1.32
> >   @@ -121,6 +121,10 @@
> >    #define SATISFY_ANY 1
> >    #define SATISFY_NOSPEC 2
> >    
> >   +/* Make sure we don't write less than 4096 bytes at any one time.
> >   + */
> >   +#define AP_MIN_BYTES_TO_WRITE  9000
> 
> That doesn't look for 4096 bytes :-)

ARGH!  I looked at that as I copied it and meant to change it.  :-)

> >...
> >   --- http_protocol.c	2000/11/18 14:43:26	1.250
> >   +++ http_protocol.c	2000/11/21 20:17:20	1.251
> >...
> >   @@ -2304,8 +2278,35 @@
> >            }
> >            r->bytes_sent += length;
> 
> One nit about this logic: it should only do an ap_bucket_read() if
> e->length == -1. We don't want to read file contents into memory.
> *ESPECIALLY* because this is before the test for AP_MIN_BYTES_TO_WRITE.
> 
> Urk. That reading logic is even worse than I thought. It could end up
> reading a 20 meg file into memory before batting an eyelash. Bogus.

I thought that was fixed already.  Hmm..  I'll look again.

> 
> >        }
> >   +
> >   +    if (r->bytes_sent < AP_MIN_BYTES_TO_WRITE) {
> >   +        ap_save_brigade(f, &ctx->saved, &b);
> >   +        return APR_SUCCESS;
> >   +    }
> 
> I think this is tying the filters together too much. Now, the C-L filter has
> knowledge that the core filter is buffering these bytes.

Ahh. but with this change, we can reduce some of the complexity over
time.  For example, if we know the c-l filter is buffering (it ALWAYS
buffers 9K at a time), why is the core buffering it too?  It shouldn't
anymore.  Plus, the c-l filter is always called before chunk, at least it
should be, so we can stop worrying about those tiny chunks being sent.

> Further, this is wrong because the brigade may have had an EOS bucket in it.
> With the above code, it will save the brigade and never get a chance to send
> it. The logic needs to include "send_it" somehow.

Need to look at that more.  I thought I got that case.

> >   +    /* We will compute a content length if:
> >   +     *     We already have all the data
> >   +     *         This is a bit confusing, because we will always buffer up
> >   +     *         to AP_MIN_BYTES_TO_WRITE, so if we get all the data while
> >   +     *         we are buffering that much data, we set the c-l.
> >   +     *  or We are in a 1.1 request and we can't chunk
> >   +     *  or This is a keepalive connection
> >   +     *         We may want to change this later to just close the connection
> >   +     */
> 
> This is happening on each sequence through the filter. It should only happen
> on the first pass through the filter.

No.  I purposefully did it on each pass of the filter, because we may
buffer the first 8.5K, then get 7.5K and an EOS on the next call.  Now we
have all the data, so we should send the c-l, even though we have more
than 9K.

> nit: indentation of "chunked" is off.
> 
> >   +        || (f->r->connection->keepalive)
> 
> In the determination for r->chunked (in ap_set_keepalive()), it compares the
> connection->keepalive value against -1.

-1 is an error condition.  0 means no-keepalive, and 1 means
keep-alive.  At least that's what the docs say.  I'll check again.

> >   +    }
> >   +    if (ctx->saved) {
> >   +        AP_BRIGADE_CONCAT(ctx->saved, b);
> >   +        b = ctx->saved;
> >        }
> 
> ctx->saved should be set to NULL so that you don't try to send it again.
> 
> Also: on all of our uses of AP_BRIGADE_CONCAT(), shouldn't we follow that
> with an ap_brigade_destroy(second_arg_to_concat) ?

Yep.  All good catches, patches coming.

Ryan

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


Mime
View raw message