You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2023/06/01 13:21:05 UTC
Re: svn commit: r1910161 - in /httpd/httpd/trunk: changes-entries/ include/ modules/http/ test/modules/http2/
On 6/1/23 2:21 PM, icing@apache.org wrote:
> Author: icing
> Date: Thu Jun 1 12:21:03 2023
> New Revision: 1910161
>
> URL: http://svn.apache.org/viewvc?rev=1910161&view=rev
> Log:
> *) core: add `final_resp_passed` flag to request_rec to allow
> ap_die() to judge if it can send out a response. Bump mmn.
> Enable test cases that check errors during response body to
> appear as error on client side.
>
>
> Added:
> httpd/httpd/trunk/changes-entries/resp_passed.txt
> Modified:
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/include/httpd.h
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/modules/http/http_request.c
> httpd/httpd/trunk/test/modules/http2/test_008_ranges.py
> httpd/httpd/trunk/test/modules/http2/test_500_proxy.py
> httpd/httpd/trunk/test/modules/http2/test_601_h2proxy_twisted.py
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1910161&r1=1910160&r2=1910161&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Jun 1 12:21:03 2023
> @@ -1265,7 +1265,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>
> if (ctx->final_status && ctx->final_header_only) {
> /* The final RESPONSE has already been sent or is in front of `bcontent`
> - * in the brigade. For a header_only respsone, remove all content buckets
> + * in the brigade. For a header_only respone, remove all content buckets
Now the typo is different :-)
> * up to the first EOS. On seeing EOS, we remove ourself and are done.
> * NOTE that `header_only` responses never generate trailes.
> */
> Modified: httpd/httpd/trunk/modules/http/http_request.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_request.c?rev=1910161&r1=1910160&r2=1910161&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_request.c (original)
> +++ httpd/httpd/trunk/modules/http/http_request.c Thu Jun 1 12:21:03 2023
> @@ -84,38 +84,25 @@ static void ap_die_r(int type, request_r
> return;
> }
>
> - 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;
> - }
Out of curiosity: You changed to using the flag instead as the above did not detect all cases correctly or because it is not
performance optimal?
> + /*
> + * if we have already passed the final response down the
> + * output filter chain, we cannot generate a second final
> + * response here.
> + */
> + if (r->final_resp_passed) {
> + return;
> + }
>
> - /*
> - * 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;
> + if (!ap_is_HTTP_VALID_RESPONSE(type)) {
> + if (type != AP_FILTER_ERROR) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
> + "Invalid response status %i", type);
> }
> else {
> - return;
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
> + "Response from AP_FILTER_ERROR");
> }
> + type = HTTP_INTERNAL_SERVER_ERROR;
> }
>
> /*
>
Regards
Rüdiger
Re: svn commit: r1910161 - in /httpd/httpd/trunk: changes-entries/ include/ modules/http/ test/modules/http2/
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/1/23 3:30 PM, Stefan Eissing via dev wrote:
>>> Modified: httpd/httpd/trunk/modules/http/http_request.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_request.c?rev=1910161&r1=1910160&r2=1910161&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/http/http_request.c (original)
>>> +++ httpd/httpd/trunk/modules/http/http_request.c Thu Jun 1 12:21:03 2023
>>> @@ -84,38 +84,25 @@ static void ap_die_r(int type, request_r
>>> return;
>>> }
>>>
>>> - 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;
>>> - }
>>
>> Out of curiosity: You changed to using the flag instead as the above did not detect all cases correctly or because it is not
>> performance optimal?
>
> It did not catch all cases any longer since the httpd header filter does not remove itself any longer. This is a result of the RESPONSE buckets changes in trunk. So when ap_die() was called during response body processing, it added a 500 body content to the response.
>
> Instead of fiddling here with filter chain expectations, I found it more sane to keep the bit at request_rec. But it can be discussed if there is a better choice.
I think the new approach is cleaner and less performance consuming. I was just curious about the motivation to do it.
Regards
Rüdiger
Re: svn commit: r1910161 - in /httpd/httpd/trunk: changes-entries/ include/ modules/http/ test/modules/http2/
Posted by Stefan Eissing via dev <de...@httpd.apache.org>.
> Am 01.06.2023 um 15:21 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 6/1/23 2:21 PM, icing@apache.org wrote:
>> Author: icing
>> Date: Thu Jun 1 12:21:03 2023
>> New Revision: 1910161
>>
>> URL: http://svn.apache.org/viewvc?rev=1910161&view=rev
>> Log:
>> *) core: add `final_resp_passed` flag to request_rec to allow
>> ap_die() to judge if it can send out a response. Bump mmn.
>> Enable test cases that check errors during response body to
>> appear as error on client side.
>>
>>
>> Added:
>> httpd/httpd/trunk/changes-entries/resp_passed.txt
>> Modified:
>> httpd/httpd/trunk/include/ap_mmn.h
>> httpd/httpd/trunk/include/httpd.h
>> httpd/httpd/trunk/modules/http/http_filters.c
>> httpd/httpd/trunk/modules/http/http_request.c
>> httpd/httpd/trunk/test/modules/http2/test_008_ranges.py
>> httpd/httpd/trunk/test/modules/http2/test_500_proxy.py
>> httpd/httpd/trunk/test/modules/http2/test_601_h2proxy_twisted.py
>>
>
>
>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1910161&r1=1910160&r2=1910161&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Jun 1 12:21:03 2023
>> @@ -1265,7 +1265,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>
>> if (ctx->final_status && ctx->final_header_only) {
>> /* The final RESPONSE has already been sent or is in front of `bcontent`
>> - * in the brigade. For a header_only respsone, remove all content buckets
>> + * in the brigade. For a header_only respone, remove all content buckets
>
> Now the typo is different :-)
Spelling error forever! \o/
>
>> * up to the first EOS. On seeing EOS, we remove ourself and are done.
>> * NOTE that `header_only` responses never generate trailes.
>> */
>
>> Modified: httpd/httpd/trunk/modules/http/http_request.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_request.c?rev=1910161&r1=1910160&r2=1910161&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http/http_request.c (original)
>> +++ httpd/httpd/trunk/modules/http/http_request.c Thu Jun 1 12:21:03 2023
>> @@ -84,38 +84,25 @@ static void ap_die_r(int type, request_r
>> return;
>> }
>>
>> - 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;
>> - }
>
> Out of curiosity: You changed to using the flag instead as the above did not detect all cases correctly or because it is not
> performance optimal?
It did not catch all cases any longer since the httpd header filter does not remove itself any longer. This is a result of the RESPONSE buckets changes in trunk. So when ap_die() was called during response body processing, it added a 500 body content to the response.
Instead of fiddling here with filter chain expectations, I found it more sane to keep the bit at request_rec. But it can be discussed if there is a better choice.
Cheers,
Stefan
>
>> + /*
>> + * if we have already passed the final response down the
>> + * output filter chain, we cannot generate a second final
>> + * response here.
>> + */
>> + if (r->final_resp_passed) {
>> + return;
>> + }
>>
>> - /*
>> - * 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;
>> + if (!ap_is_HTTP_VALID_RESPONSE(type)) {
>> + if (type != AP_FILTER_ERROR) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
>> + "Invalid response status %i", type);
>> }
>> else {
>> - return;
>> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
>> + "Response from AP_FILTER_ERROR");
>> }
>> + type = HTTP_INTERNAL_SERVER_ERROR;
>> }
>>
>> /*
>>
>
> Regards
>
> Rüdiger
>