httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@attglobal.net>
Subject Re: [PATCH] checking for failures encountered by core_output_filter
Date Fri, 01 Nov 2002 16:13:41 GMT
Justin Erenkrantz <jerenkrantz@apache.org> writes:

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

The only filter I changed was the content-length filter.

But yes I would agree that in a commit which fixes our handlers as you
mentioned we also fix the content-length filter to not worry about
c->aborted.

Note that I changed the mod_cgi handler to look at c->aborted as well
as rv after ap_pass_brigade().  In all honesty I don't know who gets
to set c->aborted and whether rv absolutely must be non-APR_SUCCESS if
aborted is set.  But I suspect that mod_cgi wouldn't have to check
both rv and c->aborted to see if an error occurred talking to the
client.

> 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.

Talk to Ryan :)

> 
> 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?

I'm kinda stuck thinking that the philosophy should be that we log
whatever was written to the client in the status line, but there are
some implementation details with that one.

> 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?

What I meant by "what is logged" is "which HTTP status code ends up in
the access log".  I didn't mean error log.  We'll always write to the
access log (i.e., we'll always run that logging hook) for any request
that we actually read, right?

>                                                               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.

One issue with that is that the filter that encounters the error
really should do the logging so that the most specific information is
available.  If some other code also logs less-specific info, then it
is all for naught.

I'll try not to think about this until Monday :)

-- 
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...

Mime
View raw message