You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Greg Stein <gs...@lyra.org> on 2000/11/21 22:44:49 UTC
C-L filter problems (was: Re: cvs commit: ...)
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 :-)
>...
> --- 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.
> }
> +
> + 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.
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.
> + /* 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.
> + if ((r->proto_num == HTTP_VERSION(1,1)
> + && !ap_find_last_token(f->r->pool,
> + apr_table_get(r->headers_out,
> + "Transfer-Encoding"),
> + "chunked"))
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.
>...
> @@ -2313,11 +2314,11 @@
> ap_save_brigade(f, &ctx->saved, &b);
> return APR_SUCCESS;
> }
> - if (ctx->saved) {
> - AP_BRIGADE_CONCAT(ctx->saved, b);
> - b = ctx->saved;
> - }
> ap_set_content_length(r, r->bytes_sent);
> + }
> + 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) ?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: C-L filter problems (was: Re: cvs commit: ...)
Posted by rb...@covalent.net.
On Tue, 21 Nov 2000, Greg Stein wrote:
> On Tue, Nov 21, 2000 at 02:35:37PM -0800, rbb@covalent.net wrote:
> > > >
> > > > 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.
> >
> > Greg, take a closer look, we already have the if (e->length == -1) logic
> > in there.
>
> Oops! My bad. The diff didn't have enough context to look at that, so I
> checked my local copy. Well... that local copy wasn't up to date. Hrmph.
>
> Sorry for the yellow flag :-)
Dude, no apologies necessary. That yellow flag made me go back and look
too. These are the joys of Open Source. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: C-L filter problems (was: Re: cvs commit: ...)
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 21, 2000 at 02:35:37PM -0800, rbb@covalent.net wrote:
> > > >...
> > > > --- 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.
>
> Greg, take a closer look, we already have the if (e->length == -1) logic
> in there.
Oops! My bad. The diff didn't have enough context to look at that, so I
checked my local copy. Well... that local copy wasn't up to date. Hrmph.
Sorry for the yellow flag :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: C-L filter problems (was: Re: cvs commit: ...)
Posted by rb...@covalent.net.
> > >...
> > > --- 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.
Greg, take a closer look, we already have the if (e->length == -1) logic
in there.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: C-L filter problems (was: Re: cvs commit: ...)
Posted by rb...@covalent.net.
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
-------------------------------------------------------------------------------