Mailing List Archive

Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/modules/http1/mod_h1test/ test/modules/http2/ test/pyhtt
On 4/4/22 1:08 PM, icing@apache.org wrote:
> Author: icing
> Date: Mon Apr 4 11:08:58 2022
> New Revision: 1899552
>
> URL: http://svn.apache.org/viewvc?rev=1899552&view=rev
> Log:
> *) mod_http: genereate HEADERS buckets for trailers
> mod_proxy: forward trailers on chunked request encoding
> test: add http/1.x test cases in pytest
>
>
> Added:
> httpd/httpd/trunk/test/modules/http1/
> httpd/httpd/trunk/test/modules/http1/__init__.py
> httpd/httpd/trunk/test/modules/http1/conftest.py
> httpd/httpd/trunk/test/modules/http1/env.py
> httpd/httpd/trunk/test/modules/http1/htdocs/
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py (with props)
> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py (with props)
> httpd/httpd/trunk/test/modules/http1/mod_h1test/
> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
> httpd/httpd/trunk/test/modules/http1/test_003_get.py
> httpd/httpd/trunk/test/modules/http1/test_004_post.py
> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
> Modified:
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/test/modules/http2/env.py
> httpd/httpd/trunk/test/pyhttpd/conf.py
> httpd/httpd/trunk/test/pyhttpd/env.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=1899552&r1=1899551&r2=1899552&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr 4 11:08:58 2022
> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
> return DONE;
> }
>
> +static apr_bucket *create_trailers_bucket(request_rec *r, apr_bucket_alloc_t *bucket_alloc)
> +{
> + if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
> + return ap_bucket_headers_create(r->trailers_out, r->pool, bucket_alloc);
> + }
> + return NULL;
> +}
> +
> typedef struct header_filter_ctx {
> int headers_sent;
> } header_filter_ctx;
> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
> conn_rec *c = r->connection;
> int header_only = (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status));
> const char *protocol = NULL;
> - apr_bucket *e;
> + apr_bucket *e, *eos = NULL;
> apr_bucket_brigade *b2;
> header_struct h;
> header_filter_ctx *ctx = f->ctx;
> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
> 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.
> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
> goto out;
> }
>
> + if (eos) {
> + /* on having seen EOS and added possible trailers, we
> + * can remove this filter.
> + */
> + e = create_trailers_bucket(r, b->bucket_alloc);
> + if (e) {
> + APR_BUCKET_INSERT_BEFORE(eos, e);
> + }
> + ap_remove_output_filter(f);
> + }
> +
> + if (ctx->headers_sent) {
> + /* we did already the stuff below, just pass on */
> + return ap_pass_brigade(f->next, b);
> + }
> +

I guess this does not work for header_only requests with trailing headers.

> /*
> * Now that we are ready to send a response, we need to combine the two
> * header field tables into a single table. If we don't do this, our

Regards

Rüdiger
Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/modules/http1/mod_h1test/ test/modules/http2/ test/pyhtt [ In reply to ]
> Am 04.04.2022 um 16:07 schrieb Ruediger Pluem <rpluem@apache.org>:
>
>
>
> On 4/4/22 1:08 PM, icing@apache.org wrote:
>> Author: icing
>> Date: Mon Apr 4 11:08:58 2022
>> New Revision: 1899552
>>
>> URL: http://svn.apache.org/viewvc?rev=1899552&view=rev
>> Log:
>> *) mod_http: genereate HEADERS buckets for trailers
>> mod_proxy: forward trailers on chunked request encoding
>> test: add http/1.x test cases in pytest
>>
>>
>> Added:
>> httpd/httpd/trunk/test/modules/http1/
>> httpd/httpd/trunk/test/modules/http1/__init__.py
>> httpd/httpd/trunk/test/modules/http1/conftest.py
>> httpd/httpd/trunk/test/modules/http1/env.py
>> httpd/httpd/trunk/test/modules/http1/htdocs/
>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py (with props)
>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py (with props)
>> httpd/httpd/trunk/test/modules/http1/mod_h1test/
>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
>> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
>> httpd/httpd/trunk/test/modules/http1/test_003_get.py
>> httpd/httpd/trunk/test/modules/http1/test_004_post.py
>> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
>> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
>> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
>> Modified:
>> httpd/httpd/trunk/modules/http/http_filters.c
>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>> httpd/httpd/trunk/test/modules/http2/env.py
>> httpd/httpd/trunk/test/pyhttpd/conf.py
>> httpd/httpd/trunk/test/pyhttpd/env.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=1899552&r1=1899551&r2=1899552&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr 4 11:08:58 2022
>> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>> return DONE;
>> }
>>
>> +static apr_bucket *create_trailers_bucket(request_rec *r, apr_bucket_alloc_t *bucket_alloc)
>> +{
>> + if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
>> + return ap_bucket_headers_create(r->trailers_out, r->pool, bucket_alloc);
>> + }
>> + return NULL;
>> +}
>> +
>> typedef struct header_filter_ctx {
>> int headers_sent;
>> } header_filter_ctx;
>> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>> conn_rec *c = r->connection;
>> int header_only = (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status));
>> const char *protocol = NULL;
>> - apr_bucket *e;
>> + apr_bucket *e, *eos = NULL;
>> apr_bucket_brigade *b2;
>> header_struct h;
>> header_filter_ctx *ctx = f->ctx;
>> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>> 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.
>> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>> goto out;
>> }
>>
>> + if (eos) {
>> + /* on having seen EOS and added possible trailers, we
>> + * can remove this filter.
>> + */
>> + e = create_trailers_bucket(r, b->bucket_alloc);
>> + if (e) {
>> + APR_BUCKET_INSERT_BEFORE(eos, e);
>> + }
>> + ap_remove_output_filter(f);
>> + }
>> +
>> + if (ctx->headers_sent) {
>> + /* we did already the stuff below, just pass on */
>> + return ap_pass_brigade(f->next, b);
>> + }
>> +
>
> I guess this does not work for header_only requests with trailing headers.

The thought came up if we want/need/may support that. Can/should a 304 have trailers if the unconditional request had?

I added the king to CC, to see if he has an opinion.

Kind Regards,
Stefan

>
>> /*
>> * Now that we are ready to send a response, we need to combine the two
>> * header field tables into a single table. If we don't do this, our
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/modules/http1/mod_h1test/ test/modules/http2/ test/pyhtt [ In reply to ]
On 4/5/22 9:13 AM, Stefan Eissing wrote:
>
>
>> Am 04.04.2022 um 16:07 schrieb Ruediger Pluem <rpluem@apache.org>:
>>
>>
>>
>> On 4/4/22 1:08 PM, icing@apache.org wrote:
>>> Author: icing
>>> Date: Mon Apr 4 11:08:58 2022
>>> New Revision: 1899552
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1899552&view=rev
>>> Log:
>>> *) mod_http: genereate HEADERS buckets for trailers
>>> mod_proxy: forward trailers on chunked request encoding
>>> test: add http/1.x test cases in pytest
>>>
>>>
>>> Added:
>>> httpd/httpd/trunk/test/modules/http1/
>>> httpd/httpd/trunk/test/modules/http1/__init__.py
>>> httpd/httpd/trunk/test/modules/http1/conftest.py
>>> httpd/httpd/trunk/test/modules/http1/env.py
>>> httpd/httpd/trunk/test/modules/http1/htdocs/
>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py (with props)
>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py (with props)
>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/
>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
>>> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
>>> httpd/httpd/trunk/test/modules/http1/test_003_get.py
>>> httpd/httpd/trunk/test/modules/http1/test_004_post.py
>>> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
>>> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
>>> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
>>> Modified:
>>> httpd/httpd/trunk/modules/http/http_filters.c
>>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>>> httpd/httpd/trunk/test/modules/http2/env.py
>>> httpd/httpd/trunk/test/pyhttpd/conf.py
>>> httpd/httpd/trunk/test/pyhttpd/env.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=1899552&r1=1899551&r2=1899552&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr 4 11:08:58 2022
>>> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>>> return DONE;
>>> }
>>>
>>> +static apr_bucket *create_trailers_bucket(request_rec *r, apr_bucket_alloc_t *bucket_alloc)
>>> +{
>>> + if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
>>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
>>> + return ap_bucket_headers_create(r->trailers_out, r->pool, bucket_alloc);
>>> + }
>>> + return NULL;
>>> +}
>>> +
>>> typedef struct header_filter_ctx {
>>> int headers_sent;
>>> } header_filter_ctx;
>>> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> conn_rec *c = r->connection;
>>> int header_only = (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status));
>>> const char *protocol = NULL;
>>> - apr_bucket *e;
>>> + apr_bucket *e, *eos = NULL;
>>> apr_bucket_brigade *b2;
>>> header_struct h;
>>> header_filter_ctx *ctx = f->ctx;
>>> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> 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.
>>> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>> goto out;
>>> }
>>>
>>> + if (eos) {
>>> + /* on having seen EOS and added possible trailers, we
>>> + * can remove this filter.
>>> + */
>>> + e = create_trailers_bucket(r, b->bucket_alloc);
>>> + if (e) {
>>> + APR_BUCKET_INSERT_BEFORE(eos, e);
>>> + }
>>> + ap_remove_output_filter(f);
>>> + }
>>> +
>>> + if (ctx->headers_sent) {
>>> + /* we did already the stuff below, just pass on */
>>> + return ap_pass_brigade(f->next, b);
>>> + }
>>> +
>>
>> I guess this does not work for header_only requests with trailing headers.
>
> The thought came up if we want/need/may support that. Can/should a 304 have trailers if the unconditional request had?

What about HEAD requests where the corresponding GET has trailers?

Regards

Rüdiger
Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/modules/http1/mod_h1test/ test/modules/http2/ test/pyhtt [ In reply to ]
> Am 05.04.2022 um 09:34 schrieb Ruediger Pluem <rpluem@apache.org>:
>
>
>
> On 4/5/22 9:13 AM, Stefan Eissing wrote:
>>
>>
>>> Am 04.04.2022 um 16:07 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>
>>>
>>>
>>> On 4/4/22 1:08 PM, icing@apache.org wrote:
>>>> Author: icing
>>>> Date: Mon Apr 4 11:08:58 2022
>>>> New Revision: 1899552
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1899552&view=rev
>>>> Log:
>>>> *) mod_http: genereate HEADERS buckets for trailers
>>>> mod_proxy: forward trailers on chunked request encoding
>>>> test: add http/1.x test cases in pytest
>>>>
>>>>
>>>> Added:
>>>> httpd/httpd/trunk/test/modules/http1/
>>>> httpd/httpd/trunk/test/modules/http1/__init__.py
>>>> httpd/httpd/trunk/test/modules/http1/conftest.py
>>>> httpd/httpd/trunk/test/modules/http1/env.py
>>>> httpd/httpd/trunk/test/modules/http1/htdocs/
>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py (with props)
>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py (with props)
>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/
>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
>>>> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
>>>> httpd/httpd/trunk/test/modules/http1/test_003_get.py
>>>> httpd/httpd/trunk/test/modules/http1/test_004_post.py
>>>> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
>>>> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
>>>> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
>>>> Modified:
>>>> httpd/httpd/trunk/modules/http/http_filters.c
>>>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>>>> httpd/httpd/trunk/test/modules/http2/env.py
>>>> httpd/httpd/trunk/test/pyhttpd/conf.py
>>>> httpd/httpd/trunk/test/pyhttpd/env.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=1899552&r1=1899551&r2=1899552&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr 4 11:08:58 2022
>>>> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>>>> return DONE;
>>>> }
>>>>
>>>> +static apr_bucket *create_trailers_bucket(request_rec *r, apr_bucket_alloc_t *bucket_alloc)
>>>> +{
>>>> + if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
>>>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
>>>> + return ap_bucket_headers_create(r->trailers_out, r->pool, bucket_alloc);
>>>> + }
>>>> + return NULL;
>>>> +}
>>>> +
>>>> typedef struct header_filter_ctx {
>>>> int headers_sent;
>>>> } header_filter_ctx;
>>>> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>> conn_rec *c = r->connection;
>>>> int header_only = (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status));
>>>> const char *protocol = NULL;
>>>> - apr_bucket *e;
>>>> + apr_bucket *e, *eos = NULL;
>>>> apr_bucket_brigade *b2;
>>>> header_struct h;
>>>> header_filter_ctx *ctx = f->ctx;
>>>> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>> 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.
>>>> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>> goto out;
>>>> }
>>>>
>>>> + if (eos) {
>>>> + /* on having seen EOS and added possible trailers, we
>>>> + * can remove this filter.
>>>> + */
>>>> + e = create_trailers_bucket(r, b->bucket_alloc);
>>>> + if (e) {
>>>> + APR_BUCKET_INSERT_BEFORE(eos, e);
>>>> + }
>>>> + ap_remove_output_filter(f);
>>>> + }
>>>> +
>>>> + if (ctx->headers_sent) {
>>>> + /* we did already the stuff below, just pass on */
>>>> + return ap_pass_brigade(f->next, b);
>>>> + }
>>>> +
>>>
>>> I guess this does not work for header_only requests with trailing headers.
>>
>> The thought came up if we want/need/may support that. Can/should a 304 have trailers if the unconditional request had?
>
> What about HEAD requests where the corresponding GET has trailers?

Looking at https://datatracker.ietf.org/doc/draft-ietf-httpbis-semantics/

ch. 15.3.5 "204 No Content": "A 204 response is terminated by the end of the header section; it
cannot contain content or trailers."


ch. 15.4.5 "304": "A 304 response is terminated by the end of the header section; it
cannot contain content or trailers."

ch. 9.3.2. "HEAD": "However, a server MAY omit header fields for which a value is
determined only while generating the content."


The first 2 are pretty clear. On HEAD responses, the server internally only discards the
content (eg reads to nothing), if the connection is not tagged for closing. If we allow
trailers in HEAD responses, this becomes unreliable. The overall connection state would
determine the response. I think we should avoid that.

In addition, for HTTP/2 HEAD responses, the body is never read by our implementation.


Related to all this, but outside the proposed changes so far: I think we should switch
to "chunked" encoding in our HTTP/1.1 responses when a "Trailers" header is set. That way
a loaded module that, for example, generates content digests could signal that
it will generate trailers.

Kind Regards,
Stefan

> Regards
>
> Rüdiger
Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/modules/http1/mod_h1test/ test/modules/http2/ test/pyhtt [ In reply to ]
On 4/5/22 9:53 AM, Stefan Eissing wrote:
>
>
>> Am 05.04.2022 um 09:34 schrieb Ruediger Pluem <rpluem@apache.org>:
>>
>>
>>
>> On 4/5/22 9:13 AM, Stefan Eissing wrote:
>>>
>>>
>>>> Am 04.04.2022 um 16:07 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>>
>>>>
>>>>
>>>> On 4/4/22 1:08 PM, icing@apache.org wrote:
>>>>> Author: icing
>>>>> Date: Mon Apr 4 11:08:58 2022
>>>>> New Revision: 1899552
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1899552&view=rev
>>>>> Log:
>>>>> *) mod_http: genereate HEADERS buckets for trailers
>>>>> mod_proxy: forward trailers on chunked request encoding
>>>>> test: add http/1.x test cases in pytest
>>>>>
>>>>>
>>>>> Added:
>>>>> httpd/httpd/trunk/test/modules/http1/
>>>>> httpd/httpd/trunk/test/modules/http1/__init__.py
>>>>> httpd/httpd/trunk/test/modules/http1/conftest.py
>>>>> httpd/httpd/trunk/test/modules/http1/env.py
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py (with props)
>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py (with props)
>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/
>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
>>>>> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_003_get.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_004_post.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
>>>>> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
>>>>> Modified:
>>>>> httpd/httpd/trunk/modules/http/http_filters.c
>>>>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>>>>> httpd/httpd/trunk/test/modules/http2/env.py
>>>>> httpd/httpd/trunk/test/pyhttpd/conf.py
>>>>> httpd/httpd/trunk/test/pyhttpd/env.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=1899552&r1=1899551&r2=1899552&view=diff
>>>>> ==============================================================================
>>>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr 4 11:08:58 2022
>>>>> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>>>>> return DONE;
>>>>> }
>>>>>
>>>>> +static apr_bucket *create_trailers_bucket(request_rec *r, apr_bucket_alloc_t *bucket_alloc)
>>>>> +{
>>>>> + if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
>>>>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
>>>>> + return ap_bucket_headers_create(r->trailers_out, r->pool, bucket_alloc);
>>>>> + }
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> typedef struct header_filter_ctx {
>>>>> int headers_sent;
>>>>> } header_filter_ctx;
>>>>> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>> conn_rec *c = r->connection;
>>>>> int header_only = (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status));
>>>>> const char *protocol = NULL;
>>>>> - apr_bucket *e;
>>>>> + apr_bucket *e, *eos = NULL;
>>>>> apr_bucket_brigade *b2;
>>>>> header_struct h;
>>>>> header_filter_ctx *ctx = f->ctx;
>>>>> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>> 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.
>>>>> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>> goto out;
>>>>> }
>>>>>
>>>>> + if (eos) {
>>>>> + /* on having seen EOS and added possible trailers, we
>>>>> + * can remove this filter.
>>>>> + */
>>>>> + e = create_trailers_bucket(r, b->bucket_alloc);
>>>>> + if (e) {
>>>>> + APR_BUCKET_INSERT_BEFORE(eos, e);
>>>>> + }
>>>>> + ap_remove_output_filter(f);
>>>>> + }
>>>>> +
>>>>> + if (ctx->headers_sent) {
>>>>> + /* we did already the stuff below, just pass on */
>>>>> + return ap_pass_brigade(f->next, b);
>>>>> + }
>>>>> +
>>>>
>>>> I guess this does not work for header_only requests with trailing headers.
>>>
>>> The thought came up if we want/need/may support that. Can/should a 304 have trailers if the unconditional request had?
>>
>> What about HEAD requests where the corresponding GET has trailers?
>
> Looking at https://datatracker.ietf.org/doc/draft-ietf-httpbis-semantics/
>
> ch. 15.3.5 "204 No Content": "A 204 response is terminated by the end of the header section; it
> cannot contain content or trailers."
>
>
> ch. 15.4.5 "304": "A 304 response is terminated by the end of the header section; it
> cannot contain content or trailers."
>
> ch. 9.3.2. "HEAD": "However, a server MAY omit header fields for which a value is
> determined only while generating the content."
>
>
> The first 2 are pretty clear. On HEAD responses, the server internally only discards the
> content (eg reads to nothing), if the connection is not tagged for closing. If we allow
> trailers in HEAD responses, this becomes unreliable. The overall connection state would
> determine the response. I think we should avoid that.
>
> In addition, for HTTP/2 HEAD responses, the body is never read by our implementation.

So the proposal is to drop possible trailers on HEAD requests, which is what the code above does?

Regards

Rüdiger
Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ test/modules/http1/htdocs/ test/modules/http1/htdocs/cgi/ test/modules/http1/htdocs/cgi/files/ test/modules/http1/mod_h1test/ test/modules/http2/ test/pyhtt [ In reply to ]
> Am 05.04.2022 um 10:20 schrieb Ruediger Pluem <rpluem@apache.org>:
>
>
>
> On 4/5/22 9:53 AM, Stefan Eissing wrote:
>>
>>
>>> Am 05.04.2022 um 09:34 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>
>>>
>>>
>>> On 4/5/22 9:13 AM, Stefan Eissing wrote:
>>>>
>>>>
>>>>> Am 04.04.2022 um 16:07 schrieb Ruediger Pluem <rpluem@apache.org>:
>>>>>
>>>>>
>>>>>
>>>>> On 4/4/22 1:08 PM, icing@apache.org wrote:
>>>>>> Author: icing
>>>>>> Date: Mon Apr 4 11:08:58 2022
>>>>>> New Revision: 1899552
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1899552&view=rev
>>>>>> Log:
>>>>>> *) mod_http: genereate HEADERS buckets for trailers
>>>>>> mod_proxy: forward trailers on chunked request encoding
>>>>>> test: add http/1.x test cases in pytest
>>>>>>
>>>>>>
>>>>>> Added:
>>>>>> httpd/httpd/trunk/test/modules/http1/
>>>>>> httpd/httpd/trunk/test/modules/http1/__init__.py
>>>>>> httpd/httpd/trunk/test/modules/http1/conftest.py
>>>>>> httpd/httpd/trunk/test/modules/http1/env.py
>>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/
>>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/
>>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/
>>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/files/empty.txt
>>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/hello.py (with props)
>>>>>> httpd/httpd/trunk/test/modules/http1/htdocs/cgi/upload.py (with props)
>>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/
>>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.c
>>>>>> httpd/httpd/trunk/test/modules/http1/mod_h1test/mod_h1test.slo
>>>>>> httpd/httpd/trunk/test/modules/http1/test_001_alive.py
>>>>>> httpd/httpd/trunk/test/modules/http1/test_003_get.py
>>>>>> httpd/httpd/trunk/test/modules/http1/test_004_post.py
>>>>>> httpd/httpd/trunk/test/modules/http1/test_005_trailers.py
>>>>>> httpd/httpd/trunk/test/modules/http1/test_006_unsafe.py
>>>>>> httpd/httpd/trunk/test/modules/http1/test_007_strict.py
>>>>>> Modified:
>>>>>> httpd/httpd/trunk/modules/http/http_filters.c
>>>>>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>>>>>> httpd/httpd/trunk/test/modules/http2/env.py
>>>>>> httpd/httpd/trunk/test/pyhttpd/conf.py
>>>>>> httpd/httpd/trunk/test/pyhttpd/env.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=1899552&r1=1899551&r2=1899552&view=diff
>>>>>> ==============================================================================
>>>>>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>>>>>> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Apr 4 11:08:58 2022
>>>>>> @@ -1312,6 +1312,15 @@ AP_DECLARE_NONSTD(int) ap_send_http_trac
>>>>>> return DONE;
>>>>>> }
>>>>>>
>>>>>> +static apr_bucket *create_trailers_bucket(request_rec *r, apr_bucket_alloc_t *bucket_alloc)
>>>>>> +{
>>>>>> + if (r->trailers_out && !apr_is_empty_table(r->trailers_out)) {
>>>>>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending trailers");
>>>>>> + return ap_bucket_headers_create(r->trailers_out, r->pool, bucket_alloc);
>>>>>> + }
>>>>>> + return NULL;
>>>>>> +}
>>>>>> +
>>>>>> typedef struct header_filter_ctx {
>>>>>> int headers_sent;
>>>>>> } header_filter_ctx;
>>>>>> @@ -1323,7 +1332,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>>> conn_rec *c = r->connection;
>>>>>> int header_only = (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status));
>>>>>> const char *protocol = NULL;
>>>>>> - apr_bucket *e;
>>>>>> + apr_bucket *e, *eos = NULL;
>>>>>> apr_bucket_brigade *b2;
>>>>>> header_struct h;
>>>>>> header_filter_ctx *ctx = f->ctx;
>>>>>> @@ -1364,6 +1373,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>>> 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.
>>>>>> @@ -1416,6 +1429,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>>>>>> goto out;
>>>>>> }
>>>>>>
>>>>>> + if (eos) {
>>>>>> + /* on having seen EOS and added possible trailers, we
>>>>>> + * can remove this filter.
>>>>>> + */
>>>>>> + e = create_trailers_bucket(r, b->bucket_alloc);
>>>>>> + if (e) {
>>>>>> + APR_BUCKET_INSERT_BEFORE(eos, e);
>>>>>> + }
>>>>>> + ap_remove_output_filter(f);
>>>>>> + }
>>>>>> +
>>>>>> + if (ctx->headers_sent) {
>>>>>> + /* we did already the stuff below, just pass on */
>>>>>> + return ap_pass_brigade(f->next, b);
>>>>>> + }
>>>>>> +
>>>>>
>>>>> I guess this does not work for header_only requests with trailing headers.
>>>>
>>>> The thought came up if we want/need/may support that. Can/should a 304 have trailers if the unconditional request had?
>>>
>>> What about HEAD requests where the corresponding GET has trailers?
>>
>> Looking at https://datatracker.ietf.org/doc/draft-ietf-httpbis-semantics/
>>
>> ch. 15.3.5 "204 No Content": "A 204 response is terminated by the end of the header section; it
>> cannot contain content or trailers."
>>
>>
>> ch. 15.4.5 "304": "A 304 response is terminated by the end of the header section; it
>> cannot contain content or trailers."
>>
>> ch. 9.3.2. "HEAD": "However, a server MAY omit header fields for which a value is
>> determined only while generating the content."
>>
>>
>> The first 2 are pretty clear. On HEAD responses, the server internally only discards the
>> content (eg reads to nothing), if the connection is not tagged for closing. If we allow
>> trailers in HEAD responses, this becomes unreliable. The overall connection state would
>> determine the response. I think we should avoid that.
>>
>> In addition, for HTTP/2 HEAD responses, the body is never read by our implementation.
>
> So the proposal is to drop possible trailers on HEAD requests, which is what the code above does?

I think that is the only sane thing to do. Because we do not (and do not want to) guarantee that content is processed for HEAD requests.

>
> Regards
>
> Rüdiger