httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@apache.org>
Subject Re: [PATCH] checking for failures encountered by core_output_filter
Date Fri, 01 Nov 2002 08:53:50 GMT
--On Thursday, October 31, 2002 4:58 PM -0500 Jeff Trawick 
<trawick@attglobal.net> wrote:

> default_handler(), which should be returning HTTP status code,
> returns whatever ap_pass_brigade() returns.  default_handler()
> would have to change too.  Any other handlers as well (In the
> thread I pointed to, Ryan claimed that 99% of handlers return
> whatever ap_pass_brigade() returned.  So I suspect that some other
> handlers need to be changed as well.)

Right, it's been a long-standing problem that this API is completely 
horked (yet another reason I'm not overjoyed with the thought of 
forcing the 2.0.43 module API on all developers until we release a 
2.4/3.0).  OtherBill just suggested to me a solution like changing 
the default_handler line from:

return ap_pass_brigade(r->output_filters, bb);

to:

rv = ap_pass_brigade(r->output_filters, bb);
if (rv != APR_SUCCESS && r->status == HTTP_OK) {
    return HTTP_INTERNAL_SERVER_ERROR;
}
else {
    return r->status;
}

That way, if a filter does set r->status for some reason, we return 
that value, otherwise we return 500.

Handlers should be returning HTTP status codes, while filters should 
be returning APR status codes.  I find myself believe that is the 
right model, because in theory (gah!), the filters could be more than 
just HTTP.  Too bad we can't use enums for one of those status codes 
and wash our hands of the whole deal by relying on type safety.  Oh, 
well.

I think your commits to check c->aborted in various filters should be 
replaced by getting core_input_filter to return APR_ECONNABORTED.

Namely the following lines in core_output_filter (server/core.c:4002) 
are wrong:

    /* No need to check for SUCCESS, we did that above. */
    if (!APR_STATUS_IS_EAGAIN(rv)) {
        c->aborted = 1;
    }

    /* The client has aborted, but the request was successful. We
     * will report success, and leave it to the access and error
     * logs to note that the connection was aborted.
     */
    return APR_SUCCESS;

I don't buy that logic at all.  Why should we be returning 
APR_SUCCESS here?  We want to signal an error, so that the filters 
stop what they are doing and exit.

Hmm.  By what I just said for default_handler, if a client aborts, 
that would mean that the handler would be returning with a 500. 
Hmmm.  Perhaps we could catch the c->aborted case in the default 
handler and just 'lie' and say that it would have been a 200.  Hmm. 
Not sure about that one.  Thoughts?

Random thought: define a new HTTP status code which means 'Client got 
the hell out of here, so we stopped early' (207??).  Remember that 
this status code is only going to be presented via the access logs, 
so we really don't need 'support' from HTTP clients.  That could 
solve our problem...

> Code playing with HTTP status codes should be analyzed to ensure
> that what is logged is what was written to the client (I guess?)
> (i.e., some subsequent error doesn't overwrite r->status or
> whatever is passed to the logger).

Do we really want to be logging if a filter has an error?  Here's 
another random thought: introduce a logging filter at the top of the 
filter chain which will write a note to the error log if the filters 
return something other than APR_SUCCESS.  This way we don't have to 
explicitly handle that in all of the handlers.

> clear as mud?

Oh, sure.  -- justin

Mime
View raw message