'Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ t'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apache-httpd-dev
Subject:    Re: svn commit: r1899552 - in /httpd/httpd/trunk: modules/http/ modules/proxy/ test/modules/http1/ t
From:       Stefan Eissing <stefan () eissing ! org>
Date:       2022-04-05 8:23:23
Message-ID: E0CC6A35-D65A-472E-9C83-0D68D4E9B8DF () eissing ! org
[Download RAW message or body]



> 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


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic