Hello!
On Fri, Jun 02, 2017 at 02:04:06AM -0700, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotrsik...@google.com>
> # Date 1490351854 25200
> # Fri Mar 24 03:37:34 2017 -0700
> # Node ID b0a910ad494158427ba102bdac71ce01d0667f72
> # Parent 716852cce9136d977b81a2d1b8b6f9fbca0dce49
> Added support for trailers in HTTP responses.
>
> Example:
>
> ngx_table_elt_t *h;
>
> h = ngx_list_push(&r->headers_out.trailers);
> if (h == NULL) {
> return NGX_ERROR;
> }
>
> ngx_str_set(&h->key, "Fun");
> ngx_str_set(&h->value, "with trailers");
> h->hash = ngx_hash_key_lc(h->key.data, h->key.len);
>
> The code above adds "Fun: with trailers" trailer to the response to
> the request with "TE: trailers" header (which indicates support for
> trailers).
Note: the "TE: trailers" requirement is no longer present in the
code.
>
> Modules that want to emit trailers must set r->expect_trailers = 1,
> otherwise they are going to be ignored.
>
> This change also adds $sent_trailer_* variables.
>
> Signed-off-by: Piotr Sikora <piotrsik...@google.com>
>
> diff -r 716852cce913 -r b0a910ad4941
> src/http/modules/ngx_http_chunked_filter_module.c
> --- a/src/http/modules/ngx_http_chunked_filter_module.c
> +++ b/src/http/modules/ngx_http_chunked_filter_module.c
> @@ -17,6 +17,8 @@ typedef struct {
>
>
> static ngx_int_t ngx_http_chunked_filter_init(ngx_conf_t *cf);
> +static ngx_chain_t *ngx_http_chunked_create_trailers(ngx_http_request_t *r,
> + ngx_uint_t emit_crlf);
>
>
> static ngx_http_module_t ngx_http_chunked_filter_module_ctx = {
> @@ -69,28 +71,31 @@ ngx_http_chunked_header_filter(ngx_http_
> return ngx_http_next_header_filter(r);
> }
>
> - if (r->headers_out.content_length_n == -1) {
> + clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
> +
> + if (clcf->chunked_transfer_encoding && r->expect_trailers) {
> + ngx_http_clear_content_length(r);
> + r->chunked = 1;
> +
This code results in using chunked encoding for HTTP/1.0 when
trailers are expected. Such behaviour is explicitly forbidden by
the HTTP/1.1 specification, and will very likely result in
problems (we've seen lots of such problems with broken backends
when there were no HTTP/1.1 support in the proxy module).
Something like this should be a better solution:
if (r->headers_out.content_length_n == -1
|| r->expect_trailers)
{
clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
if (r->http_version >= NGX_HTTP_VERSION_11
&& clcf->chunked_transfer_encoding)
{
if (r->expect_trailers) {
ngx_http_clear_content_length(r);
}
r->chunked = 1;
ctx = ngx_pcalloc(r->pool,
sizeof(ngx_http_chunked_filter_ctx_t));
if (ctx == NULL) {
return NGX_ERROR;
}
ngx_http_set_ctx(r, ctx, ngx_http_chunked_filter_module);
} else if (r->headers_out.content_length_n == -1) {
r->keepalive = 0;
}
}
> + } else if (r->headers_out.content_length_n == -1) {
> if (r->http_version < NGX_HTTP_VERSION_11) {
> r->keepalive = 0;
>
> + } else if (clcf->chunked_transfer_encoding) {
> + r->chunked = 1;
> +
> } else {
> - clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
> + r->keepalive = 0;
> + }
> + }
>
> - if (clcf->chunked_transfer_encoding) {
> - r->chunked = 1;
> + if (r->chunked) {
> + ctx = ngx_pcalloc(r->pool, sizeof(ngx_http_chunked_filter_ctx_t));
> + if (ctx == NULL) {
> + return NGX_ERROR;
> + }
>
> - ctx = ngx_pcalloc(r->pool,
> - sizeof(ngx_http_chunked_filter_ctx_t));
> - if (ctx == NULL) {
> - return NGX_ERROR;
> - }
> -
> - ngx_http_set_ctx(r, ctx, ngx_http_chunked_filter_module);
> -
> - } else {
> - r->keepalive = 0;
> - }
> - }
> + ngx_http_set_ctx(r, ctx, ngx_http_chunked_filter_module);
> }
>
> return ngx_http_next_header_filter(r);
> @@ -179,28 +184,38 @@ ngx_http_chunked_body_filter(ngx_http_re
> }
>
> if (cl->buf->last_buf) {
> - tl = ngx_chain_get_free_buf(r->pool, &ctx->free);
> - if (tl == NULL) {
> - return NGX_ERROR;
> +
> + if (r->expect_trailers) {
> + tl = ngx_http_chunked_create_trailers(r, size ? 1 : 0);
See the previous thread about the r->expect_trailers test here.
> +
> + } else {
> + tl = NULL;
> }
>
> - b = tl->buf;
> + if (tl == NULL) {
> + tl = ngx_chain_get_free_buf(r->pool, &ctx->free);
> + if (tl == NULL) {
> + return NGX_ERROR;
> + }
Instead of providing two separate code paths for "with trailer
headers" and "without trailer headers", it might be better and
more readable to generate last-chunk in one function regardless of
whether trailer headers are present or not.
It will also make error handling better: as of now, an allocation
error in ngx_http_chunked_create_trailers() will result in "no
trailers" code path being tried instead of returning an
unconditional error.
>
> - b->tag = (ngx_buf_tag_t) &ngx_http_chunked_filter_module;
> - b->temporary = 0;
> - b->memory = 1;
> - b->last_buf = 1;
> - b->pos = (u_char *) CRLF "0" CRLF CRLF;
> - b->last = b->pos + 7;
> + b = tl->buf;
>
> - cl->buf->last_buf = 0;
> + b->tag = (ngx_buf_tag_t) &ngx_http_chunked_filter_module;
> + b->temporary = 0;
> + b->memory = 1;
> + b->last_buf = 1;
> + b->pos = (u_char *) CRLF "0" CRLF CRLF;
> + b->last = b->pos + 7;
> +
> + cl->buf->last_buf = 0;
> +
> + if (size == 0) {
> + b->pos += 2;
> + }
> + }
>
> *ll = tl;
>
> - if (size == 0) {
> - b->pos += 2;
> - }
> -
> } else if (size > 0) {
> tl = ngx_chain_get_free_buf(r->pool, &ctx->free);
> if (tl == NULL) {
> @@ -230,6 +245,118 @@ ngx_http_chunked_body_filter(ngx_http_re
> }
>
>
> +static ngx_chain_t *
> +ngx_http_chunked_create_trailers(ngx_http_request_t *r, ngx_uint_t emit_crlf)
> +{
> + size_t len;
> + ngx_buf_t *b;
> + ngx_uint_t i;
> + ngx_chain_t *cl;
> + ngx_list_part_t *part;
> + ngx_table_elt_t *header;
> + ngx_http_chunked_filter_ctx_t *ctx;
> +
> + len = 0;
> +
> + part = &r->headers_out.trailers.part;
> + header = part->elts;
> +
> + for (i = 0; /* void */; i++) {
> +
> + if (i >= part->nelts) {
> + if (part->next == NULL) {
> + break;
> + }
> +
> + part = part->next;
> + header = part->elts;
> + i = 0;
> + }
> +
> + if (header[i].hash == 0) {
> + continue;
> + }
> +
> + len += header[i].key.len + sizeof(": ") - 1
> + + header[i].value.len + sizeof(CRLF) - 1;
> + }
> +
> + if (len == 0) {
> + return NULL;
See above, generating here an empty last-chunk might be better
approach.
> + }
> +
> + if (emit_crlf) {
> + len += sizeof(CRLF) - 1;
> + }
It might worth to always add CRLF, and skip it in the
ngx_http_chunked_body_filter() filter if not needed.
> +
> + len += sizeof("0") - 1 + sizeof(CRLF) - 1 + sizeof(CRLF) - 1;
There is no need to write sizeof() so many times, just
len += sizeof(CRLF "0" CRLF CRLF) - 1;
would be enough.
> +
> + ctx = ngx_http_get_module_ctx(r, ngx_http_chunked_filter_module);
> +
> + cl = ngx_chain_get_free_buf(r->pool, &ctx->free);
> + if (cl == NULL) {
> + return NULL;
> + }
> +
> + b = cl->buf;
> +
> + b->tag = (ngx_buf_tag_t) &ngx_http_chunked_filter_module;
> + b->temporary = 0;
> + b->memory = 1;
> + b->last_buf = 1;
> +
> + b->start = ngx_palloc(r->pool, len);
> + if (b->start == NULL) {
> + return NULL;
> + }
> +
> + b->end = b->last + len;
> + b->pos = b->start;
> + b->last = b->start;
> +
> + if (emit_crlf) {
> + *b->last++ = CR; *b->last++ = LF;
> + }
> +
> + *b->last++ = '0';
> + *b->last++ = CR; *b->last++ = LF;
> +
> + part = &r->headers_out.trailers.part;
> + header = part->elts;
> +
> + for (i = 0; /* void */; i++) {
> +
> + if (i >= part->nelts) {
> + if (part->next == NULL) {
> + break;
> + }
> +
> + part = part->next;
> + header = part->elts;
> + i = 0;
> + }
> +
> + if (header[i].hash == 0) {
> + continue;
> + }
> +
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "http trailer: \"%V: %V\"",
> + &header[i].key, &header[i].value);
> +
> + b->last = ngx_copy(b->last, header[i].key.data, header[i].key.len);
> + *b->last++ = ':'; *b->last++ = ' ';
> +
> + b->last = ngx_copy(b->last, header[i].value.data,
> header[i].value.len);
> + *b->last++ = CR; *b->last++ = LF;
> + }
> +
> + *b->last++ = CR; *b->last++ = LF;
> +
> + return cl;
> +}
> +
> +
> static ngx_int_t
> ngx_http_chunked_filter_init(ngx_conf_t *cf)
> {
> diff -r 716852cce913 -r b0a910ad4941 src/http/ngx_http_core_module.c
> --- a/src/http/ngx_http_core_module.c
> +++ b/src/http/ngx_http_core_module.c
> @@ -2484,6 +2484,13 @@ ngx_http_subrequest(ngx_http_request_t *
> return NGX_ERROR;
> }
>
> + if (ngx_list_init(&sr->headers_out.trailers, r->pool, 4,
> + sizeof(ngx_table_elt_t))
> + != NGX_OK)
> + {
> + return NGX_ERROR;
> + }
> +
> cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module);
> sr->main_conf = cscf->ctx->main_conf;
> sr->srv_conf = cscf->ctx->srv_conf;
> diff -r 716852cce913 -r b0a910ad4941 src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -562,6 +562,14 @@ ngx_http_create_request(ngx_connection_t
> return NULL;
> }
>
> + if (ngx_list_init(&r->headers_out.trailers, r->pool, 4,
> + sizeof(ngx_table_elt_t))
> + != NGX_OK)
> + {
> + ngx_destroy_pool(r->pool);
> + return NULL;
> + }
> +
> r->ctx = ngx_pcalloc(r->pool, sizeof(void *) * ngx_http_max_module);
> if (r->ctx == NULL) {
> ngx_destroy_pool(r->pool);
> diff -r 716852cce913 -r b0a910ad4941 src/http/ngx_http_request.h
> --- a/src/http/ngx_http_request.h
> +++ b/src/http/ngx_http_request.h
> @@ -252,6 +252,7 @@ typedef struct {
>
> typedef struct {
> ngx_list_t headers;
> + ngx_list_t trailers;
>
> ngx_uint_t status;
> ngx_str_t status_line;
> @@ -514,6 +515,7 @@ struct ngx_http_request_s {
> unsigned pipeline:1;
> unsigned chunked:1;
> unsigned header_only:1;
> + unsigned expect_trailers:1;
> unsigned keepalive:1;
> unsigned lingering_close:1;
> unsigned discard_body:1;
> diff -r 716852cce913 -r b0a910ad4941 src/http/ngx_http_variables.c
> --- a/src/http/ngx_http_variables.c
> +++ b/src/http/ngx_http_variables.c
> @@ -38,6 +38,8 @@ static ngx_int_t ngx_http_variable_unkno
> ngx_http_variable_value_t *v, uintptr_t data);
> static ngx_int_t ngx_http_variable_unknown_header_out(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data);
> +static ngx_int_t ngx_http_variable_unknown_trailer_out(ngx_http_request_t *r,
> + ngx_http_variable_value_t *v, uintptr_t data);
> static ngx_int_t ngx_http_variable_request_line(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data);
> static ngx_int_t ngx_http_variable_cookie(ngx_http_request_t *r,
> @@ -365,6 +367,9 @@ static ngx_http_variable_t ngx_http_cor
> { ngx_string("sent_http_"), NULL, ngx_http_variable_unknown_header_out,
> 0, NGX_HTTP_VAR_PREFIX, 0 },
>
> + { ngx_string("sent_trailer_"), NULL,
> ngx_http_variable_unknown_trailer_out,
> + 0, NGX_HTTP_VAR_PREFIX, 0 },
> +
> { ngx_string("cookie_"), NULL, ngx_http_variable_cookie,
> 0, NGX_HTTP_VAR_PREFIX, 0 },
>
> @@ -934,6 +939,16 @@ ngx_http_variable_unknown_header_out(ngx
> }
>
>
> +static ngx_int_t
> +ngx_http_variable_unknown_trailer_out(ngx_http_request_t *r,
> + ngx_http_variable_value_t *v, uintptr_t data)
> +{
> + return ngx_http_variable_unknown_header(v, (ngx_str_t *) data,
> + &r->headers_out.trailers.part,
> + sizeof("sent_trailer_") - 1);
> +}
> +
> +
> ngx_int_t
> ngx_http_variable_unknown_header(ngx_http_variable_value_t *v, ngx_str_t
> *var,
> ngx_list_part_t *part, size_t prefix)
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
