Mailing List Archive

Re: svn commit: r1899648 - in /httpd/httpd/trunk: changes-entries/core_response_buckets.txt include/mod_core.h modules/http/http_core.c modules/http/http_filters.c modules/http/http_protocol.c server/protocol.c
On 4/7/22 12:41 PM, icing@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.

> }
> - }
>
> - if (!ctx->headers_sent && !check_headers(r)) {
> - /* We may come back here from ap_die() below,
> - * so clear anything from this response.
> - */
> - apr_table_clear(r->headers_out);
> - apr_table_clear(r->err_headers_out);
> - apr_brigade_cleanup(b);
> + if (respb) {
> + ap_bucket_response *resp = respb->data;
> + if (resp->status >= 200 || resp->status == 101) {

Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?

> + /* Someone is passing the final response, remember it
> + * so we no longer generate one. */
> + ctx->final_status = resp->status;
> + ctx->final_header_only = AP_STATUS_IS_HEADER_ONLY(resp->status);
> + }
> + }
>

> @@ -1430,148 +1306,15 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
> }
>
> 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);
> - }
> -
> - /*
> - * 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
> - * later attempts to set or unset a given fieldname might be bypassed.
> - */
> - if (!apr_is_empty_table(r->err_headers_out)) {
> - r->headers_out = apr_table_overlay(r->pool, r->err_headers_out,
> - r->headers_out);
> - }
> -
> - /*
> - * Remove the 'Vary' header field if the client can't handle it.
> - * Since this will have nasty effects on HTTP/1.1 caches, force
> - * the response into HTTP/1.0 mode.
> - *
> - * Note: the force-response-1.0 should come before the call to
> - * basic_http_header_check()
> - */
> - if (apr_table_get(r->subprocess_env, "force-no-vary") != NULL) {
> - apr_table_unset(r->headers_out, "Vary");
> - r->proto_num = HTTP_VERSION(1,0);
> - apr_table_setn(r->subprocess_env, "force-response-1.0", "1");
> - }
> - else {
> - fixup_vary(r);
> - }
> -
> - /*
> - * Now remove any ETag response header field if earlier processing
> - * says so (such as a 'FileETag None' directive).
> - */
> - if (apr_table_get(r->notes, "no-etag") != NULL) {
> - apr_table_unset(r->headers_out, "ETag");
> - }
> -
> - /* determine the protocol and whether we should use keepalives. */
> - basic_http_header_check(r, &protocol);
> - ap_set_keepalive(r);
> -
> - if (AP_STATUS_IS_HEADER_ONLY(r->status)) {
> - apr_table_unset(r->headers_out, "Transfer-Encoding");
> - apr_table_unset(r->headers_out, "Content-Length");
> - r->content_type = r->content_encoding = NULL;
> - r->content_languages = NULL;
> - r->clength = r->chunked = 0;
> - }
> - else if (r->chunked) {
> - apr_table_mergen(r->headers_out, "Transfer-Encoding", "chunked");
> - apr_table_unset(r->headers_out, "Content-Length");
> - }
> -
> - ctype = ap_make_content_type(r, r->content_type);
> - if (ctype) {
> - apr_table_setn(r->headers_out, "Content-Type", ctype);
> - }
> -
> - if (r->content_encoding) {
> - apr_table_setn(r->headers_out, "Content-Encoding",
> - r->content_encoding);
> - }
> -
> - if (!apr_is_empty_array(r->content_languages)) {
> - int i;
> - char *token;
> - char **languages = (char **)(r->content_languages->elts);
> - const char *field = apr_table_get(r->headers_out, "Content-Language");
> -
> - while (field && (token = ap_get_list_item(r->pool, &field)) != NULL) {
> - for (i = 0; i < r->content_languages->nelts; ++i) {
> - if (!ap_cstr_casecmp(token, languages[i]))
> - break;
> - }
> - if (i == r->content_languages->nelts) {
> - *((char **) apr_array_push(r->content_languages)) = token;
> - }
> - }
> -
> - field = apr_array_pstrcat(r->pool, r->content_languages, ',');
> - apr_table_setn(r->headers_out, "Content-Language", field);
> - }
> -
> - /*
> - * Control cachability for non-cacheable responses if not already set by
> - * some other part of the server configuration.
> - */
> - if (r->no_cache && !apr_table_get(r->headers_out, "Expires")) {
> - char *date = apr_palloc(r->pool, APR_RFC822_DATE_LEN);
> - ap_recent_rfc822_date(date, r->request_time);
> - apr_table_addn(r->headers_out, "Expires", date);
> - }
> -
> - b2 = apr_brigade_create(r->pool, c->bucket_alloc);
> - basic_http_header(r, b2, protocol);
> -
> - h.pool = r->pool;
> - h.bb = b2;
> -
> - send_all_header_fields(&h, r);
> -
> - terminate_header(b2);
> -
> - if (header_only) {
> - e = APR_BRIGADE_LAST(b);
> - if (e != APR_BRIGADE_SENTINEL(b) && APR_BUCKET_IS_EOS(e)) {
> - APR_BUCKET_REMOVE(e);
> - APR_BRIGADE_INSERT_TAIL(b2, e);
> - ap_remove_output_filter(f);
> - }
> - apr_brigade_cleanup(b);
> - }
> -
> - rv = ap_pass_brigade(f->next, b2);
> - apr_brigade_cleanup(b2);
> - ctx->headers_sent = 1;
> -
> - if (rv != APR_SUCCESS || header_only) {
> - goto out;
> - }
> -
> - r->sent_bodyct = 1; /* Whatever follows is real body stuff... */
> -
> - if (r->chunked) {
> - /* We can't add this filter until we have already sent the headers.
> - * If we add it before this point, then the headers will be chunked
> - * as well, and that is just wrong.
> - */
> - ap_add_output_filter("CHUNK", NULL, r, r->connection);
> + else if (ctx->final_status == 101) {

Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?

> + /* switching protocol, whatever comes next is not HTTP/1.x */
> + ap_remove_output_filter(f);
> }
>
> rv = ap_pass_brigade(f->next, b);
> @@ -1989,3 +1732,351 @@ apr_status_t ap_http_outerror_filter(ap_
>
> return ap_pass_brigade(f->next, b);
> }
> +
> +
> +/* fill "bb" with a barebones/initial HTTP/1.x response header */
> +static void h1_append_response_head(request_rec *r,
> + ap_bucket_response *resp,
> + const char *protocol,
> + apr_bucket_brigade *bb)
> +{
> + const char *date = NULL;
> + const char *server = NULL;
> + const char *status_line;
> + struct iovec vec[4];
> +
> + if (r->assbackwards) {
> + /* there are no headers to send */
> + return;
> + }
> +
> + /* Output the HTTP/1.x Status-Line and the Date and Server fields */
> + if (resp->reason) {
> + status_line = apr_psprintf(r->pool, "%d %s", resp->status, resp->reason);
> + }
> + else {
> + status_line = ap_get_status_line_ex(r->pool, resp->status);
> + }
> +
> + vec[0].iov_base = (void *)protocol;
> + vec[0].iov_len = strlen(protocol);
> + vec[1].iov_base = (void *)" ";
> + vec[1].iov_len = sizeof(" ") - 1;
> + vec[2].iov_base = (void *)(status_line);
> + vec[2].iov_len = strlen(status_line);
> + vec[3].iov_base = (void *)CRLF;
> + vec[3].iov_len = sizeof(CRLF) - 1;
> +#if APR_CHARSET_EBCDIC
> + {
> + char *tmp;
> + apr_size_t len;
> + tmp = apr_pstrcatv(r->pool, vec, 4, &len);
> + ap_xlate_proto_to_ascii(tmp, len);
> + apr_brigade_write(bb, NULL, NULL, tmp, len);
> + }
> +#else
> + apr_brigade_writev(bb, NULL, NULL, vec, 4);
> +#endif
> +
> + date = apr_table_get(resp->headers, "Date");
> + server = apr_table_get(resp->headers, "Server");
> + if (date) {
> + /* We always write that as first, just because we
> + * always did and some quirky clients might rely on that.
> + */
> + ap_h1_append_header(bb, r->pool, "Date", date);
> + apr_table_unset(resp->headers, "Date");
> + }
> + if (server) {
> + /* We always write that second, just because we
> + * always did and some quirky clients might rely on that.
> + */
> + ap_h1_append_header(bb, r->pool, "Server", server);
> + apr_table_unset(resp->headers, "Server");
> + }
> +
> + if (APLOGrtrace3(r)) {
> + ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
> + "Response sent with status %d%s",
> + r->status,
> + APLOGrtrace4(r) ? ", headers:" : "");
> +
> + /*
> + * Date and Server are less interesting, use TRACE5 for them while
> + * using TRACE4 for the other headers.
> + */
> + if (date)
> + ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r, " Date: %s",
> + date);
> + if (server)
> + ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r, " Server: %s",
> + server);
> + }
> +}
> +
> +typedef struct h1_response_ctx {
> + int final_response_sent; /* strict: a response status >= 200 was sent */
> + int discard_body; /* the response is header only, discard body */
> + apr_bucket_brigade *tmpbb;
> +} h1_response_ctx;
> +
> +AP_CORE_DECLARE_NONSTD(apr_status_t) ap_h1_response_out_filter(ap_filter_t *f,
> + apr_bucket_brigade *b)
> +{
> + request_rec *r = f->r;
> + conn_rec *c = r->connection;
> + apr_bucket *e, *next = NULL;
> + h1_response_ctx *ctx = f->ctx;
> + apr_status_t rv = APR_SUCCESS;
> + core_server_config *conf = ap_get_core_module_config(r->server->module_config);
> + int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
> +
> + AP_DEBUG_ASSERT(!r->main);
> +
> + if (!ctx) {
> + ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
> + }
> +
> + for (e = APR_BRIGADE_FIRST(b);
> + e != APR_BRIGADE_SENTINEL(b);
> + e = next)
> + {
> + next = APR_BUCKET_NEXT(e);
> +
> + if (APR_BUCKET_IS_METADATA(e)) {
> +
> + if (APR_BUCKET_IS_EOS(e)) {
> + if (!ctx->final_response_sent) {
> + /* should not happen. do we generate a 500 here? */
> + }
> + ap_remove_output_filter(f);
> + goto pass;
> + }
> + else if (AP_BUCKET_IS_RESPONSE(e)) {
> + ap_bucket_response *resp = e->data;
> +
> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> + "ap_http1_response_out_filter seeing response bucket status=%d",
> + resp->status);
> + if (strict && resp->status < 100) {
> + /* error, not a valid http status */
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10386)
> + "ap_http1_response_out_filter seeing headers "
> + "status=%d in strict mode",
> + resp->status);
> + rv = AP_FILTER_ERROR;
> + goto cleanup;
> + }
> + else if (ctx->final_response_sent) {
> + /* already sent the final response for the request.
> + */
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10387)
> + "ap_http1_response_out_filter seeing headers "
> + "status=%d after final response already sent",
> + resp->status);
> + rv = AP_FILTER_ERROR;
> + goto cleanup;
> + }
> + else {
> + /* a response status to transcode, might be final or interim
> + */
> + const char *proto = AP_SERVER_PROTOCOL;
> +
> + ctx->final_response_sent = (resp->status >= 200)
> + || (!strict && resp->status < 100);
> + ctx->discard_body = ctx->final_response_sent &&
> + (r->header_only || AP_STATUS_IS_HEADER_ONLY(resp->status));
> +
> + if (!ctx->tmpbb) {
> + ctx->tmpbb = apr_brigade_create(r->pool, c->bucket_alloc);
> + }
> + if (next != APR_BRIGADE_SENTINEL(b)) {
> + apr_brigade_split_ex(b, next, ctx->tmpbb);
> + }
> +
> + if (ctx->final_response_sent) {
> + ap_h1_set_keepalive(r, resp);
> +
> + if (AP_STATUS_IS_HEADER_ONLY(resp->status)) {
> + apr_table_unset(resp->headers, "Transfer-Encoding");
> + }
> + else if (r->chunked) {
> + apr_table_mergen(resp->headers, "Transfer-Encoding", "chunked");
> + apr_table_unset(resp->headers, "Content-Length");
> + }
> + }
> +
> + /* kludge around broken browsers when indicated by force-response-1.0
> + */
> + if (r->proto_num == HTTP_VERSION(1,0)
> + && apr_table_get(r->subprocess_env, "force-response-1.0")) {
> + r->connection->keepalive = AP_CONN_CLOSE;
> + proto = "HTTP/1.0";
> + }
> + h1_append_response_head(r, resp, proto, b);
> + ap_h1_append_headers(b, r, resp->headers);
> + ap_h1_terminate_header(b);
> + apr_bucket_delete(e);
> +
> + if (ctx->final_response_sent && r->chunked) {
> + /* We can't add this filter until we have already sent the headers.
> + * If we add it before this point, then the headers will be chunked
> + * as well, and that is just wrong.
> + */
> + rv = ap_pass_brigade(f->next, b);
> + apr_brigade_cleanup(b);
> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, rv, r,
> + "ap_http1_response_out_filter passed response"
> + ", add CHUNK filter");
> + if (APR_SUCCESS != rv) {
> + apr_brigade_cleanup(ctx->tmpbb);
> + goto cleanup;
> + }
> + ap_add_output_filter("CHUNK", NULL, r, r->connection);
> + }
> +
> + APR_BRIGADE_CONCAT(b, ctx->tmpbb);
> +
> + if (resp->status == 101) {

Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?

> + /* switched to another protocol, get out of the way */
> + AP_DEBUG_ASSERT(!r->chunked);
> + ap_remove_output_filter(f);
> + goto pass;
> + }
> + }
> + }
> + }
> + else if (!ctx->final_response_sent && strict) {
> + /* data buckets before seeing the final response are in error.
> + */
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10390)
> + "ap_http1_response_out_filter seeing data before headers, %ld bytes ",
> + (long)e->length);
> + rv = AP_FILTER_ERROR;
> + goto cleanup;
> + }
> + else if (ctx->discard_body) {
> + apr_bucket_delete(e);
> + }
> + }
> +
> +pass:
> + rv = ap_pass_brigade(f->next, b);
> +cleanup:
> + return rv;
> +}
> +

Regards

Rüdiger
Re: svn commit: r1899648 - in /httpd/httpd/trunk: changes-entries/core_response_buckets.txt include/mod_core.h modules/http/http_core.c modules/http/http_filters.c modules/http/http_protocol.c server/protocol.c [ In reply to ]
> Am 13.04.2022 um 17:16 schrieb Ruediger Pluem <rpluem@apache.org>:
>
>
>
> On 4/7/22 12:41 PM, icing@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.

Seems I should change the new implementation to also do that. For H2, not seeing
a RESPONSE would be a stream error and reset the stream. Which seems fine.

>> }
>> - }
>>
>> - if (!ctx->headers_sent && !check_headers(r)) {
>> - /* We may come back here from ap_die() below,
>> - * so clear anything from this response.
>> - */
>> - apr_table_clear(r->headers_out);
>> - apr_table_clear(r->err_headers_out);
>> - apr_brigade_cleanup(b);
>> + if (respb) {
>> + ap_bucket_response *resp = respb->data;
>> + if (resp->status >= 200 || resp->status == 101) {
>
> Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?
>
>> + /* Someone is passing the final response, remember it
>> + * so we no longer generate one. */
>> + ctx->final_status = resp->status;
>> + ctx->final_header_only = AP_STATUS_IS_HEADER_ONLY(resp->status);
>> + }
>> + }
>>
>
>> @@ -1430,148 +1306,15 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>> }
>>
>> 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);
>> - }
>> -
>> - /*
>> - * 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
>> - * later attempts to set or unset a given fieldname might be bypassed.
>> - */
>> - if (!apr_is_empty_table(r->err_headers_out)) {
>> - r->headers_out = apr_table_overlay(r->pool, r->err_headers_out,
>> - r->headers_out);
>> - }
>> -
>> - /*
>> - * Remove the 'Vary' header field if the client can't handle it.
>> - * Since this will have nasty effects on HTTP/1.1 caches, force
>> - * the response into HTTP/1.0 mode.
>> - *
>> - * Note: the force-response-1.0 should come before the call to
>> - * basic_http_header_check()
>> - */
>> - if (apr_table_get(r->subprocess_env, "force-no-vary") != NULL) {
>> - apr_table_unset(r->headers_out, "Vary");
>> - r->proto_num = HTTP_VERSION(1,0);
>> - apr_table_setn(r->subprocess_env, "force-response-1.0", "1");
>> - }
>> - else {
>> - fixup_vary(r);
>> - }
>> -
>> - /*
>> - * Now remove any ETag response header field if earlier processing
>> - * says so (such as a 'FileETag None' directive).
>> - */
>> - if (apr_table_get(r->notes, "no-etag") != NULL) {
>> - apr_table_unset(r->headers_out, "ETag");
>> - }
>> -
>> - /* determine the protocol and whether we should use keepalives. */
>> - basic_http_header_check(r, &protocol);
>> - ap_set_keepalive(r);
>> -
>> - if (AP_STATUS_IS_HEADER_ONLY(r->status)) {
>> - apr_table_unset(r->headers_out, "Transfer-Encoding");
>> - apr_table_unset(r->headers_out, "Content-Length");
>> - r->content_type = r->content_encoding = NULL;
>> - r->content_languages = NULL;
>> - r->clength = r->chunked = 0;
>> - }
>> - else if (r->chunked) {
>> - apr_table_mergen(r->headers_out, "Transfer-Encoding", "chunked");
>> - apr_table_unset(r->headers_out, "Content-Length");
>> - }
>> -
>> - ctype = ap_make_content_type(r, r->content_type);
>> - if (ctype) {
>> - apr_table_setn(r->headers_out, "Content-Type", ctype);
>> - }
>> -
>> - if (r->content_encoding) {
>> - apr_table_setn(r->headers_out, "Content-Encoding",
>> - r->content_encoding);
>> - }
>> -
>> - if (!apr_is_empty_array(r->content_languages)) {
>> - int i;
>> - char *token;
>> - char **languages = (char **)(r->content_languages->elts);
>> - const char *field = apr_table_get(r->headers_out, "Content-Language");
>> -
>> - while (field && (token = ap_get_list_item(r->pool, &field)) != NULL) {
>> - for (i = 0; i < r->content_languages->nelts; ++i) {
>> - if (!ap_cstr_casecmp(token, languages[i]))
>> - break;
>> - }
>> - if (i == r->content_languages->nelts) {
>> - *((char **) apr_array_push(r->content_languages)) = token;
>> - }
>> - }
>> -
>> - field = apr_array_pstrcat(r->pool, r->content_languages, ',');
>> - apr_table_setn(r->headers_out, "Content-Language", field);
>> - }
>> -
>> - /*
>> - * Control cachability for non-cacheable responses if not already set by
>> - * some other part of the server configuration.
>> - */
>> - if (r->no_cache && !apr_table_get(r->headers_out, "Expires")) {
>> - char *date = apr_palloc(r->pool, APR_RFC822_DATE_LEN);
>> - ap_recent_rfc822_date(date, r->request_time);
>> - apr_table_addn(r->headers_out, "Expires", date);
>> - }
>> -
>> - b2 = apr_brigade_create(r->pool, c->bucket_alloc);
>> - basic_http_header(r, b2, protocol);
>> -
>> - h.pool = r->pool;
>> - h.bb = b2;
>> -
>> - send_all_header_fields(&h, r);
>> -
>> - terminate_header(b2);
>> -
>> - if (header_only) {
>> - e = APR_BRIGADE_LAST(b);
>> - if (e != APR_BRIGADE_SENTINEL(b) && APR_BUCKET_IS_EOS(e)) {
>> - APR_BUCKET_REMOVE(e);
>> - APR_BRIGADE_INSERT_TAIL(b2, e);
>> - ap_remove_output_filter(f);
>> - }
>> - apr_brigade_cleanup(b);
>> - }
>> -
>> - rv = ap_pass_brigade(f->next, b2);
>> - apr_brigade_cleanup(b2);
>> - ctx->headers_sent = 1;
>> -
>> - if (rv != APR_SUCCESS || header_only) {
>> - goto out;
>> - }
>> -
>> - r->sent_bodyct = 1; /* Whatever follows is real body stuff... */
>> -
>> - if (r->chunked) {
>> - /* We can't add this filter until we have already sent the headers.
>> - * If we add it before this point, then the headers will be chunked
>> - * as well, and that is just wrong.
>> - */
>> - ap_add_output_filter("CHUNK", NULL, r, r->connection);
>> + else if (ctx->final_status == 101) {
>
> Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?

The HTTP status numbers are burned into my cortex, but I agree that
using the constant gives better readability. Will change.
>
>> + /* switching protocol, whatever comes next is not HTTP/1.x */
>> + ap_remove_output_filter(f);
>> }
>>
>> rv = ap_pass_brigade(f->next, b);
>> @@ -1989,3 +1732,351 @@ apr_status_t ap_http_outerror_filter(ap_
>>
>> return ap_pass_brigade(f->next, b);
>> }
>> +
>> +
>> +/* fill "bb" with a barebones/initial HTTP/1.x response header */
>> +static void h1_append_response_head(request_rec *r,
>> + ap_bucket_response *resp,
>> + const char *protocol,
>> + apr_bucket_brigade *bb)
>> +{
>> + const char *date = NULL;
>> + const char *server = NULL;
>> + const char *status_line;
>> + struct iovec vec[4];
>> +
>> + if (r->assbackwards) {
>> + /* there are no headers to send */
>> + return;
>> + }
>> +
>> + /* Output the HTTP/1.x Status-Line and the Date and Server fields */
>> + if (resp->reason) {
>> + status_line = apr_psprintf(r->pool, "%d %s", resp->status, resp->reason);
>> + }
>> + else {
>> + status_line = ap_get_status_line_ex(r->pool, resp->status);
>> + }
>> +
>> + vec[0].iov_base = (void *)protocol;
>> + vec[0].iov_len = strlen(protocol);
>> + vec[1].iov_base = (void *)" ";
>> + vec[1].iov_len = sizeof(" ") - 1;
>> + vec[2].iov_base = (void *)(status_line);
>> + vec[2].iov_len = strlen(status_line);
>> + vec[3].iov_base = (void *)CRLF;
>> + vec[3].iov_len = sizeof(CRLF) - 1;
>> +#if APR_CHARSET_EBCDIC
>> + {
>> + char *tmp;
>> + apr_size_t len;
>> + tmp = apr_pstrcatv(r->pool, vec, 4, &len);
>> + ap_xlate_proto_to_ascii(tmp, len);
>> + apr_brigade_write(bb, NULL, NULL, tmp, len);
>> + }
>> +#else
>> + apr_brigade_writev(bb, NULL, NULL, vec, 4);
>> +#endif
>> +
>> + date = apr_table_get(resp->headers, "Date");
>> + server = apr_table_get(resp->headers, "Server");
>> + if (date) {
>> + /* We always write that as first, just because we
>> + * always did and some quirky clients might rely on that.
>> + */
>> + ap_h1_append_header(bb, r->pool, "Date", date);
>> + apr_table_unset(resp->headers, "Date");
>> + }
>> + if (server) {
>> + /* We always write that second, just because we
>> + * always did and some quirky clients might rely on that.
>> + */
>> + ap_h1_append_header(bb, r->pool, "Server", server);
>> + apr_table_unset(resp->headers, "Server");
>> + }
>> +
>> + if (APLOGrtrace3(r)) {
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
>> + "Response sent with status %d%s",
>> + r->status,
>> + APLOGrtrace4(r) ? ", headers:" : "");
>> +
>> + /*
>> + * Date and Server are less interesting, use TRACE5 for them while
>> + * using TRACE4 for the other headers.
>> + */
>> + if (date)
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r, " Date: %s",
>> + date);
>> + if (server)
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r, " Server: %s",
>> + server);
>> + }
>> +}
>> +
>> +typedef struct h1_response_ctx {
>> + int final_response_sent; /* strict: a response status >= 200 was sent */
>> + int discard_body; /* the response is header only, discard body */
>> + apr_bucket_brigade *tmpbb;
>> +} h1_response_ctx;
>> +
>> +AP_CORE_DECLARE_NONSTD(apr_status_t) ap_h1_response_out_filter(ap_filter_t *f,
>> + apr_bucket_brigade *b)
>> +{
>> + request_rec *r = f->r;
>> + conn_rec *c = r->connection;
>> + apr_bucket *e, *next = NULL;
>> + h1_response_ctx *ctx = f->ctx;
>> + apr_status_t rv = APR_SUCCESS;
>> + core_server_config *conf = ap_get_core_module_config(r->server->module_config);
>> + int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
>> +
>> + AP_DEBUG_ASSERT(!r->main);
>> +
>> + if (!ctx) {
>> + ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
>> + }
>> +
>> + for (e = APR_BRIGADE_FIRST(b);
>> + e != APR_BRIGADE_SENTINEL(b);
>> + e = next)
>> + {
>> + next = APR_BUCKET_NEXT(e);
>> +
>> + if (APR_BUCKET_IS_METADATA(e)) {
>> +
>> + if (APR_BUCKET_IS_EOS(e)) {
>> + if (!ctx->final_response_sent) {
>> + /* should not happen. do we generate a 500 here? */
>> + }
>> + ap_remove_output_filter(f);
>> + goto pass;
>> + }
>> + else if (AP_BUCKET_IS_RESPONSE(e)) {
>> + ap_bucket_response *resp = e->data;
>> +
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
>> + "ap_http1_response_out_filter seeing response bucket status=%d",
>> + resp->status);
>> + if (strict && resp->status < 100) {
>> + /* error, not a valid http status */
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10386)
>> + "ap_http1_response_out_filter seeing headers "
>> + "status=%d in strict mode",
>> + resp->status);
>> + rv = AP_FILTER_ERROR;
>> + goto cleanup;
>> + }
>> + else if (ctx->final_response_sent) {
>> + /* already sent the final response for the request.
>> + */
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10387)
>> + "ap_http1_response_out_filter seeing headers "
>> + "status=%d after final response already sent",
>> + resp->status);
>> + rv = AP_FILTER_ERROR;
>> + goto cleanup;
>> + }
>> + else {
>> + /* a response status to transcode, might be final or interim
>> + */
>> + const char *proto = AP_SERVER_PROTOCOL;
>> +
>> + ctx->final_response_sent = (resp->status >= 200)
>> + || (!strict && resp->status < 100);
>> + ctx->discard_body = ctx->final_response_sent &&
>> + (r->header_only || AP_STATUS_IS_HEADER_ONLY(resp->status));
>> +
>> + if (!ctx->tmpbb) {
>> + ctx->tmpbb = apr_brigade_create(r->pool, c->bucket_alloc);
>> + }
>> + if (next != APR_BRIGADE_SENTINEL(b)) {
>> + apr_brigade_split_ex(b, next, ctx->tmpbb);
>> + }
>> +
>> + if (ctx->final_response_sent) {
>> + ap_h1_set_keepalive(r, resp);
>> +
>> + if (AP_STATUS_IS_HEADER_ONLY(resp->status)) {
>> + apr_table_unset(resp->headers, "Transfer-Encoding");
>> + }
>> + else if (r->chunked) {
>> + apr_table_mergen(resp->headers, "Transfer-Encoding", "chunked");
>> + apr_table_unset(resp->headers, "Content-Length");
>> + }
>> + }
>> +
>> + /* kludge around broken browsers when indicated by force-response-1.0
>> + */
>> + if (r->proto_num == HTTP_VERSION(1,0)
>> + && apr_table_get(r->subprocess_env, "force-response-1.0")) {
>> + r->connection->keepalive = AP_CONN_CLOSE;
>> + proto = "HTTP/1.0";
>> + }
>> + h1_append_response_head(r, resp, proto, b);
>> + ap_h1_append_headers(b, r, resp->headers);
>> + ap_h1_terminate_header(b);
>> + apr_bucket_delete(e);
>> +
>> + if (ctx->final_response_sent && r->chunked) {
>> + /* We can't add this filter until we have already sent the headers.
>> + * If we add it before this point, then the headers will be chunked
>> + * as well, and that is just wrong.
>> + */
>> + rv = ap_pass_brigade(f->next, b);
>> + apr_brigade_cleanup(b);
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, rv, r,
>> + "ap_http1_response_out_filter passed response"
>> + ", add CHUNK filter");
>> + if (APR_SUCCESS != rv) {
>> + apr_brigade_cleanup(ctx->tmpbb);
>> + goto cleanup;
>> + }
>> + ap_add_output_filter("CHUNK", NULL, r, r->connection);
>> + }
>> +
>> + APR_BRIGADE_CONCAT(b, ctx->tmpbb);
>> +
>> + if (resp->status == 101) {
>
> Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?
>
>> + /* switched to another protocol, get out of the way */
>> + AP_DEBUG_ASSERT(!r->chunked);
>> + ap_remove_output_filter(f);
>> + goto pass;
>> + }
>> + }
>> + }
>> + }
>> + else if (!ctx->final_response_sent && strict) {
>> + /* data buckets before seeing the final response are in error.
>> + */
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10390)
>> + "ap_http1_response_out_filter seeing data before headers, %ld bytes ",
>> + (long)e->length);
>> + rv = AP_FILTER_ERROR;
>> + goto cleanup;
>> + }
>> + else if (ctx->discard_body) {
>> + apr_bucket_delete(e);
>> + }
>> + }
>> +
>> +pass:
>> + rv = ap_pass_brigade(f->next, b);
>> +cleanup:
>> + return rv;
>> +}
>> +
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1899648 - in /httpd/httpd/trunk: changes-entries/core_response_buckets.txt include/mod_core.h modules/http/http_core.c modules/http/http_filters.c modules/http/http_protocol.c server/protocol.c [ In reply to ]
On 4/14/22 10:18 AM, Stefan Eissing wrote:
>
>
>> Am 13.04.2022 um 17:16 schrieb Ruediger Pluem <rpluem@apache.org>:
>>
>>
>>
>> On 4/7/22 12:41 PM, icing@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