That is, not only when a hook/handler returns AP_FILTER_ERROR.
Graham (initially) and I made some changes to trunk (backport proposed
in r1665737) so that no internal module should return anything but
AP_FILTER_ERROR in this case, but we can't do much for third-party
modules.
That way ap_die() could become a safe gard against unintentional
double responses (hence HRS) for the same request, in any case since
it is always called at the end.
Something like:
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1665647)
+++ modules/http/http_request.c (working copy)
@@ -75,6 +75,7 @@ static void update_r_in_filters(ap_filter_t *f,
static void ap_die_r(int type, request_rec *r, int recursive_error)
{
+ ap_filter_t *next;
char *custom_response;
request_rec *r_1st_err = r;
@@ -83,38 +84,34 @@ static void ap_die_r(int type, request_rec *r, int
return;
}
+ /*
+ * Check if we still have the ap_http_header_filter in place. If
+ * this is the case we should not ignore the error here because
+ * it means that we have not sent any response at all. Otherwise,
+ * don't send two responses for the same request.
+ */
+ next = r->output_filters;
+ while (next && (next->frec != ap_http_header_filter_handle)) {
+ next = next->next;
+ }
+ if (!next) {
+ return;
+ }
+
+ /*
+ * AP_FILTER_ERROR or any invalid status, send an internal server
+ * error instead.
+ */
if (!ap_is_HTTP_VALID_RESPONSE(type)) {
- ap_filter_t *next;
-
- /*
- * Check if we still have the ap_http_header_filter in place. If
- * this is the case we should not ignore the error here because
- * it means that we have not sent any response at all and never
- * will. This is bad. Sent an internal server error instead.
- */
- next = r->output_filters;
- while (next && (next->frec != ap_http_header_filter_handle)) {
- next = next->next;
- }
-
- /*
- * If next != NULL then we left the while above because of
- * next->frec == ap_http_header_filter
- */
- if (next) {
- if (type != AP_FILTER_ERROR) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
- "Invalid response status %i", type);
- }
- else {
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
- "Response from AP_FILTER_ERROR");
- }
- type = HTTP_INTERNAL_SERVER_ERROR;
- }
- else {
- return;
- }
+ if (type != AP_FILTER_ERROR) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
+ "Invalid response status %i", type);
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
+ "Response from AP_FILTER_ERROR");
+ }
+ type = HTTP_INTERNAL_SERVER_ERROR;
}
/*
--
This is not a great cycles loss since this path is not a fast one
already (!OK && !DONE => ap_send_error_response() or
ap_internal_redirect()), and the inconditional loop is not really
heavy...
Thoughts?
Regards,
Yann.
