httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@covalent.net>
Subject Re: cvs commit: httpd-2.0/server/mpm/perchild perchild.c
Date Tue, 08 May 2001 14:36:40 GMT

On Tue, 8 May 2001, Greg Stein wrote:

> On Sun, May 06, 2001 at 11:27:14PM -0000, rbb@apache.org wrote:
> > rbb         01/05/06 16:27:14
> >
> >   Modified:    include  util_filter.h
> >                modules/experimental mod_case_filter_in.c mod_charset_lite.c
> >                         mod_ext_filter.c
> >                modules/http http_protocol.c http_request.c mod_core.h
> >                modules/mappers mod_alias.c
> >                modules/tls mod_tls.c
> >                server   core.c protocol.c util_filter.c
> >                server/mpm/perchild perchild.c
> >   Log:
> >   Back out the recent change to ap_get_brigade, to make it use indirection
> >   again.  The problem is that the amount of data read from the network,
> >   is not necessarily the amount of data returned from the filters.  It is
> >   possible for input filters to add bytes to the data read from the network.
> >
> >   To fix the original bug, I just removed the line from ap_get_client_block
> >   that decremented r->remaining, we allow the http_filter to do that for
> >   us.
>
> This was premature, Ryan. I don't think you should have done this. My change
> exposed further problems. It didn't create them.

It wasn't premature, because what you did broke the code, so that some of
my stuff didn't work.  And, you did it without specifying any future steps
to fix it.

> The simple problem is that r->remaining is handled at the wrong level. It
> should be handled closer to the network, before a filter monkeys with the
> number of bytes available.

Currently there is no such filter.  If we add one, it will require a new
filter type.

> Content-Length describes a network length. Therefore, it should be near the
> network. A filter that increases the number of bytes will appear "above"
> that, so the app will see more bytes. But C-L still operates down near the
> network.
>
> Second, there is no reason for the "apr_size_t *nbytes" indirection. The
> number of bytes read is in the brigade. It doesn't need to be returned. Even
> worse, some of the follow-on discussion seems to imply that you think
> "certain, magic filters" will be the only ones to adjust that value. I don't
> buy the magic.

This only works if you finish what you have described.  Had I known what
you were proposing, I probably would have finished it for you, but I
didn't.  And, when I commented that your change was incorrect, I waited a
day or two and didn't hear anything, so I reverted the code so that it
would at least work.

> ap_get_client_block() calls ap_get_brigade(2k) to get 2k of data. That goes
> down through the filter chain. The bottom-most *request* input filter (not
> connection!) is processing the Content-Length. It says 200, so it ignores
> the 2k that was passed in and asks the next filter for 200 bytes. That gets
> returned and passed back up the filter stack. In the input chain, above the
> C-L processor is a gzip filter which uncompresses the brigade to 400 bytes.
> That is then returned to my app -- 400 bytes.
>
> C-L is handled properly, decompression is handled, and my app gets the data
> appropriately.
>
> So...
>
> 1) the indirection on the prototype should go away (which I did)
> 2) the C-L handling needs to move from ap_get_client_block() down to a
>    low-level request input filter.
>
> The problem that you observed was because (2) wasn't done; not because of my
> patch. The correct solution would have been to fix (2), *not* to back out
> the work that I did.

This is all well and good, but it only works if you explain #2 when
committing #1, because otherwise nobody knows what your end-design is.

Ryan

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


Mime
View raw message