Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <456C9AAA.3070903@apache.org>
Date: Tue, 28 Nov 2006 21:23:06 +0100
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.13) Gecko/20060417
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: svn commit: r480135 -
 /httpd/httpd/trunk/modules/http/http_filters.c
References: <20061128173656.CB1341A9846@eris.apache.org>
 <20061128180508.GC17309@redhat.com>
In-Reply-To: <20061128180508.GC17309@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit


On 11/28/2006 07:05 PM, Joe Orton wrote:
> On Tue, Nov 28, 2006 at 05:36:55PM -0000, Jim Jagielski wrote:
> 
>>Author: jim
>>Date: Tue Nov 28 09:36:45 2006
>>New Revision: 480135
>>
>>URL: http://svn.apache.org/viewvc?view=rev&rev=480135
>>Log:
>>Apply patch for PR 41056 (19954) to fix chunk
>>filter. Now flushes work better.
> 
> 
> This looks wrong to me.  When reading the CRLF in state == BODY_CHUNK 
> with block == _NONBLOCK, the initial ap_get_brigade() may return EAGAIN, 
> and then the code will fall into the generic error handling branch:
> 
>                 /* Detect chunksize error (such as overflow) */
>                 if (rv != APR_SUCCESS || ctx->remaining < 0) {

Agreed. This is bad.

> 
> The API use is weird too.  This code presumes that the next filter 
> indicates read-would-block only by returning SUCCESS with an empty 
> brigade.  Yet it signals the same thing to the downstream filter only by 
> failing with EAGAIN!

Agreed.

The lines 1447 - 1450 of mod_proxy_http.c tell us

                    /* ap_get_brigade will return success with an empty brigade
                     * for a non-blocking read which would block: */
                    if (APR_STATUS_IS_EAGAIN(rv)
                        || (rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb))) {
                        /* flush to the client and switch to blocking mode */

but actually they also check for the EAGAIN case. What is the correct contract of
the input filters in the case that a non blocking read would block?

1. Return APR_SUCCESS and an empty brigade
2. Return EAGAIN
3. Choose 1. or 2. as you like.

Or should a filter be generous in what it accepts as a signal for a possible blocking read (1. / 2.)
and strict in what it returns (either 1. or 2., whatever is correct).

Regards

Rüdiger