You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2006/11/28 19:05:09 UTC
Re: svn commit: r480135 - /httpd/httpd/trunk/modules/http/http_filters.c
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) {
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!
joe
Re: svn commit: r480135 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Ruediger Pluem <rp...@apache.org>.
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