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: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.)
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...
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.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).
clear as mud?
Oh, sure. -- justin
