httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jus...@erenkrantz.com>
Subject Re: AP_MODE_EATCRLF considered indigestible
Date Fri, 15 Apr 2005 16:06:56 GMT
On Fri, Apr 15, 2005 at 10:16:29AM -0500, Rici Lake wrote:
> Perhaps I wasn't being clear. AP_MODE_EATCRLF is not a renaming
> of AP_MODE_GETLINE; it is a renaming of AP_MODE_PEEK. (The renaming
> happened some three years ago in revision 92928, obviously I'm not
> al tanto.) To be more precise, it's a renaming of what the behaviour
> of AP_MODE_PEEK was.
> 
> The documented behaviour of AP_MODE_EATCRLF is:
> /** The filter should implicitly eat any CRLF pairs that it sees. */

Remember that this is for older browsers which send extra CRLFs after each
request.

> The actual behaviour (at least in the only implementation of it in the 
> tree) is somewhat more complex:
> 
> 1) Regardless of the readtype, it only reads non-blocking
> 2) Similar to MODE_SPECULATIVE, it retains data for subsequent reads 
> (but see below)
> 3) It does not, however, ever return any data.
> 4) It returns APR_SUCCESS only if data is immediately available. In 
> determining whether or not data is immediately available, it ignores 
> ([CR]?[LF})+ (but with a bug as noted in my first message)
> 5) If the only data immediately available is ignored line-endings as 
> per (4), these line endings are not retained for subsequent reads. 
> (Obviously, this cannot be relied upon to delete leading CR?LF's, since 
> the scan only applies to currently available data.)
> 
> My argument is that this is extremely idiosyncratic behaviour and has 
> no place in a reasonable interface design. It seems to me that the only 
> consumer of AP_MODE_EATCRLF could just as well use AP_MODE_SPECULATIVE 
> (which is implemented by the only provider which implements 
> AP_MODE_EATCRLF) to do what it wants to do. Consequently, 
> AP_MODE_EATCRLF could just be honourably dispatched to the retirement 
> home for awkward interfaces, and imho we'd all be better off.

Yes, PEEK could now be rewritten using the speculative reads.  When I renamed
PEEK and added speculative reads, there was a lot of other more important and
lower-hanging fruit to deal with.  ;-)

> The following (as-yet-untested) code should do the trick. It is 
> somewhat longer than the existing check_pipeline_flush but that needs 
> to be balanced against the removal of an equal-sized chunk of 
> server/core_filters.c and the end of AP_MODE_EATCRLF:
> 
> static void check_pipeline_flush(request rec *r)
> {
>     apr_bucket *e
>     apr_bucket_brigade *bb;
>     conn_rec *c = r->connection;
>     bb = apr_brigade_create(r->pool, c->bucket_alloc);
>     if (c->keepalive != AP_CONN_CLOSE) {
>         /* The 8 is entirely arbitrary. 4 is probably sufficient */
>         if (ap_get_brigade(r->input_filters, bb, AP_MODE_SPECULATIVE,
>                            APR_NONBLOCK_READ, 8) == APR_SUCCESS) {

The problem with this code is that it'll only read 8 characters worth of
CRLFs.  We'd probably want to retry the reads until we don't get more CRLF
pairs.  It's not determinate how many extra CRLFs we will get.

So, yes, I'd be fine with removing EATCRLF for 2.2 if you provide a tested
patch.  ;-)  -- justin

Mime
View raw message