Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Date: Sun, 23 Sep 2001 21:21:25 -0700
From: Greg Stein <gstein@lyra.org>
To: dev@httpd.apache.org, rbb@covalent.net
Cc: Justin Erenkrantz <jerenkrantz@ebuilt.com>
Subject: Re: [PATCH] Take 2 of the http filter rewrite
Message-ID: <20010923212124.X4050@lyra.org>
References: <20010923185819.M6756@ebuilt.com>
 <20010924033401.6918F46DFC@koj.rkbloom.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2i
In-Reply-To: <20010924033401.6918F46DFC@koj.rkbloom.net>;
 from rbb@covalent.net on Sun, Sep 23, 2001 at 08:34:00PM -0700

On Sun, Sep 23, 2001 at 08:34:00PM -0700, Ryan Bloom wrote:
> On Sunday 23 September 2001 06:58 pm, Justin Erenkrantz wrote:
>...
> > Other significant changes is that the HTTP_IN (ap_http_filter) is
> > now a request-level filter rather than a connection-level filter.
> > This makes a lot more sense to me.

+1

>...
> I don't see how this works at all for pipelined requests.  You moved the HTTP_IN
> filter from a connection filter to a request filter. But, the only connection filter we
> have is the CORE_iN filter.  All that filter does, is to pass the socket up to the
> HTTP_IN filter. The problem is that HTTP_IN will be destroyed in between requests,
> so the second request will be lost.  I am continuing to read the code, so I may see
> what I am missing, but in the mean time, Justin, if you could explain that to me,
> it would make my review go a lot faster.

There is a create_request hook which inserts the HTTP_IN filter when the
request is created.

This seems like a good idea: each instantiation of the HTTP_IN filter is for
*one* request and one request only. We can be much more sure that state is
not spanning requests.

Even better, this enables things like the Upgrade: header (RFC 2616,
S14.42). One request specifies an upgrade, and the next request should be
handled by a different protocol.

[ of course, that still won't work since we'll continue inserting the
  HTTP_IN filter for each request, but making HTTP_IN a request filter takes
  us a step in the right direction. ]

Along the mantra of "an input filter should not return more than is asked
for", we can also be sure that the HTTP_IN filter is going to receive
exactly the amount of data that it asked for, and no more. Thus, the next
request will still be sitting in the next filter down.


Okay. Now all that said. Some random notes about the current code:

* CORE_IN is returning a new socket bucket each time. That socket bucket
  goes into a brigade associated with the request, so it gets destroyed with
  the request. Not really a problem since the actual socket is cleared by a
  pool, not by the bucket. When the next request asks for more data, it will
  get a new socket bucket. I don't think this is The Right Thing, but until
  we pass an "amount requested" parameter through the input filter chain,
  this will work quite fine.

* The uses of ap_getline are destined to eventual failure, which I've noted
  before. They always read from the top of the filter stack -- even if you
  have a mid-level filter (HTTP_IN) trying to read a line(!). This is solved
  again by passing "amount requested" down thru the input chain, where one
  of the "amounts" is a qualified constant saying "one line". We support
  that read-request mode through a brigade function to read a line into a
  pool-allocated buffer, into a provided buffer, or maybe into a new brigade
  (e.g. split a brigade at the newline point, and return the initial
  brigade).


I need to review the code some more. As Justin pointed out, the massive
change to http_protocol.c makes that hard to review. Other notes may be
coming soon...

Cheers,
-g

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