On 4/14/22 10:18 AM, Stefan Eissing wrote:
>
>
>> Am 13.04.2022 um 17:16 schrieb Ruediger Pluem <rpl...@apache.org>:
>>
>>
>>
>> On 4/7/22 12:41 PM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Thu Apr 7 10:41:46 2022
>>> New Revision: 1899648
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1899648&view=rev
>>> Log:
>>> *) core/mod_http: use RESPONSE meta buckets and a new HTTP/1.x specific
>>> filter to send responses through the output filter chain.
>>> Specifically: the HTTP_HEADER output filter and
>>> ap_send_interim_response()
>>> create a RESPONSE bucket and no longer are concerned with HTTP/1.x
>>> serialization.
>>> A new HTTP1_RESPONSE_OUT transcode filter writes the proper HTTP/1.x
>>> bytes when dealing with a RESPONSE bucket. That filter installs itself
>>> on the pre_read_request hook when the connection has protocol
>>> 'http/1.1'.
>>>
>>>
>>> Added:
>>> httpd/httpd/trunk/changes-entries/core_response_buckets.txt
>>> Modified:
>>> httpd/httpd/trunk/include/mod_core.h
>>> httpd/httpd/trunk/modules/http/http_core.c
>>> httpd/httpd/trunk/modules/http/http_filters.c
>>> httpd/httpd/trunk/modules/http/http_protocol.c
>>> httpd/httpd/trunk/server/protocol.c
>>>
>>
>>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>>> URL:
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899648&r1=1899647&r2=1899648&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Apr 7 10:41:46 2022
>>
>>> @@ -1364,62 +1178,124 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> return rv;
>>> }
>>> }
>>> -
>>> - for (e = APR_BRIGADE_FIRST(b);
>>> - e != APR_BRIGADE_SENTINEL(b);
>>> - e = APR_BUCKET_NEXT(e))
>>> - {
>>> - if (AP_BUCKET_IS_ERROR(e) && !eb) {
>>> - eb = e->data;
>>> - continue;
>>> - }
>>> - if (APR_BUCKET_IS_EOS(e)) {
>>> - if (!eos) eos = e;
>>> - continue;
>>> - }
>>> - /*
>>> - * If we see an EOC bucket it is a signal that we should get out
>>> - * of the way doing nothing.
>>> + else {
>>> + /* Determine if it is time to insert the response bucket for
>>> + * the request. Request handlers just write content or an EOS
>>> + * and that needs to take the current state of request_rec to
>>> + * send on status and headers as a response bucket.
>>> + *
>>> + * But we also send interim responses (as response buckets)
>>> + * through this filter and that must not trigger generating
>>> + * an additional response bucket.
>>> + *
>>> + * Waiting on a DATA/ERROR/EOS bucket alone is not enough,
>>> + * unfortunately, as some handlers trigger response generation
>>> + * by just writing a FLUSH (see mod_lua's websocket for example).
>>> */
>>> - if (AP_BUCKET_IS_EOC(e)) {
>>> - ap_remove_output_filter(f);
>>> - return ap_pass_brigade(f->next, b);
>>> + for (e = APR_BRIGADE_FIRST(b);
>>> + e != APR_BRIGADE_SENTINEL(b) && !trigger;
>>> + e = APR_BUCKET_NEXT(e))
>>> + {
>>> + if (AP_BUCKET_IS_RESPONSE(e)) {
>>> + /* remember the last one if there are many. */
>>> + respb = e;
>>> + }
>>> + else if (APR_BUCKET_IS_FLUSH(e)) {
>>> + /* flush without response bucket triggers */
>>> + if (!respb) trigger = e;
>>> + }
>>> + else if (APR_BUCKET_IS_EOS(e)) {
>>> + trigger = e;
>>> + }
>>> + else if (AP_BUCKET_IS_ERROR(e)) {
>>> + /* Need to handle this below via ap_die() */
>>> + break;
>>> + }
>>> + else {
>>> + /* First content bucket, always triggering the response.*/
>>> + trigger = e;
>>> + }
>>
>> Why don't we remove ourselves any longer if we hit an EOC bucket and pass
>> stuff on immediately?
>> If we want to handle this HTTP protocol agnostic here we need to ensure that
>> the EOC bucket
>> triggers the immediate removal of the ap_h1_response_out_filter once the
>> ap_h1_response_out_filter
>> sees it without putting any headers on the wire.
>> I am not sure what the appropriate behaviour would be in the HTTP > 1.1 case.
>
> Valid point. The previous filter implementation removed itself immediately on
> encountering an EOC, regardless of where it appeared in the brigade.
>
> The current implementation inserts, if not done previously, a RESPONSE bucket.
> Afterwards it checks on EOC, remove itself and passes on the brigade.
>
> The behaviour differs, if an EOC is sent *without* a final RESPONSE having
> been
> created before that. The question is what the intended behaviour of passing an
> EOC is. Reading the comment in mod_proxy_http.c:1096, it seems that the
> intention really
> is to NOT generate a response.
Correct. The idea is that when the backend just closed the connection after we
send the request
we do the same with our client: Just closing the connection without any
response provided that we
had a keepalive connection to the frontend. This comes across to our client as
if it hit a race
between sending its request to us and a keepalive closure from our side.
I am aware that this is HTTP/1.1 specific. In the light of the separation you
do in these patches
I am fine if the ap_h1_response_out_filter acts accordingly and just does not
process a RESPONSE bucket in
case there is an EOC bucket as well. This would allow other protocol versions
to adopt to their needs.
Regards
Rüdiger
