Re: [PATCH 1 of 3] Added support for trailers in HTTP responses

[Piotr Sikora via nginx-devel]

Hey Maxim,

> I've tried this as well, and decided that "if (len ==
> sizeof(...))" is slightly more readable, and also produces smaller
> patch to your code.   No strict preference though, feel free to
> use any variant you think is better.

I've ended up using "if (len == 0) { ... }" in the end, but applied
rest of you changes. Thanks!

Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 1 of 3] Added support for trailers in HTTP responses

[Piotr Sikora via nginx-devel]

# HG changeset patch
# User Piotr Sikora 
# Date 1490351854 25200
#  Fri Mar 24 03:37:34 2017 -0700
# Node ID 07a5d26b49f04425ff54cc998f885aa987b7823f
# Parent  e6f399a176e7cae0fa08f1183d31315bce3b9ecb
Added support for trailers in HTTP responses.

Example:

   ngx_table_elt_t  *h;

   h = ngx_list_push(>headers_out.trailers);
   if (h == NULL) {
   return NGX_ERROR;
   }

   ngx_str_set(>key, "Fun");
   ngx_str_set(>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.

Modules that want to emit trailers must set r->expect_trailers = 1
in header filter, otherwise they might not be emitted for HTTP/1.1
responses that aren't already chunked.

This change also adds $sent_trailer_* variables.

Signed-off-by: Piotr Sikora 

diff -r e6f399a176e7 -r 07a5d26b49f0 
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_http_chunked_filter_ctx_t *ctx);
 
 
 static ngx_http_module_t  ngx_http_chunked_filter_module_ctx = {
@@ -69,27 +71,29 @@ ngx_http_chunked_header_filter(ngx_http_
 return ngx_http_next_header_filter(r);
 }
 
-if (r->headers_out.content_length_n == -1) {
-if (r->http_version < NGX_HTTP_VERSION_11) {
+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 {
-clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
-
-if (clcf->chunked_transfer_encoding) {
-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 {
-r->keepalive = 0;
-}
 }
 }
 
@@ -179,26 +183,17 @@ ngx_http_chunked_body_filter(ngx_http_re
 }
 
 if (cl->buf->last_buf) {
-tl = ngx_chain_get_free_buf(r->pool, >free);
+tl = ngx_http_chunked_create_trailers(r, ctx);
 if (tl == NULL) {
 return NGX_ERROR;
 }
 
-b = tl->buf;
-
-b->tag = (ngx_buf_tag_t) _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;
 
 *ll = tl;
 
 if (size == 0) {
-b->pos += 2;
+tl->buf->pos += 2;
 }
 
 } else if (size > 0) {
@@ -230,6 +225,109 @@ ngx_http_chunked_body_filter(ngx_http_re
 }
 
 
+static ngx_chain_t *
+ngx_http_chunked_create_trailers(ngx_http_request_t *r,
+ngx_http_chunked_filter_ctx_t *ctx)
+{
+size_tlen;
+ngx_buf_t*b;
+ngx_uint_ti;
+ngx_chain_t  *cl;
+ngx_list_part_t  *part;
+ngx_table_elt_t  *header;
+
+len = 0;
+
+part = >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;
+}
+
+cl = ngx_chain_get_free_buf(r->pool, >free);
+if (cl == NULL) {
+return NULL;
+}
+
+b = cl->buf;
+
+b->tag = (ngx_buf_tag_t) _http_chunked_filter_module;
+b->temporary = 0;
+b->memory = 1;
+b->last_buf = 1;
+
+if (len == 0) {
+b->pos = (u_char *) CRLF "0" CRLF CRLF;
+b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1;
+return cl;
+}
+
+len += sizeof(CRLF "0" CRLF CRLF) - 1;
+
+b->pos = ngx_palloc(r->pool, len);
+if 

Re: [PATCH 1 of 3] Added support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Mon, Jun 05, 2017 at 09:56:03PM -0700, Piotr Sikora via nginx-devel wrote:

> Hey Maxim,
> 
> > I would prefer to preserve the typical code path (when there are no
> > trailers) without an extra allocation.  It looks like it would be
> > as trivail as:
> >
> > @@ -273,14 +273,18 @@ ngx_http_chunked_create_trailers(ngx_htt
> >  b->memory = 1;
> >  b->last_buf = 1;
> >
> > +if (len == sizeof(CRLF "0" CRLF CRLF) - 1) {
> > +b->pos = (u_char *) CRLF "0" CRLF CRLF;
> > +b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1;
> > +return cl;
> > +}
> 
> Sounds good, but the if statement reads a bit weird.
> 
> What about this instead, even though it might be a bit more expensive?
> 
> @@ -236,7 +236,7 @@ ngx_http_chunked_create_trailers(ngx_http_request_t *r,
>  ngx_list_part_t  *part;
>  ngx_table_elt_t  *header;
> 
> -len = sizeof(CRLF "0" CRLF CRLF) - 1;
> +len = 0;
> 
>  part = >headers_out.trailers.part;
>  header = part->elts;
> @@ -273,12 +273,14 @@ ngx_http_chunked_create_trailers(ngx_http_request_t *r,
>  b->memory = 1;
>  b->last_buf = 1;
> 
> -if (len == sizeof(CRLF "0" CRLF CRLF) - 1) {
> +if (len == 0) {
>  b->pos = (u_char *) CRLF "0" CRLF CRLF;
>  b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1;
>  return cl;
>  }
> 
> +len += sizeof(CRLF "0" CRLF CRLF) - 1;
> +
>  b->pos = ngx_palloc(r->pool, len);
>  if (b->pos == NULL) {
>  return NULL;

I've tried this as well, and decided that "if (len == 
sizeof(...))" is slightly more readable, and also produces smaller 
patch to your code.   No strict preference though, feel free to 
use any variant you think is better.

-- 
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 3] Added support for trailers in HTTP responses

[Piotr Sikora via nginx-devel]

Hey Maxim,

> I would prefer to preserve the typical code path (when there are no
> trailers) without an extra allocation.  It looks like it would be
> as trivail as:
>
> @@ -273,14 +273,18 @@ ngx_http_chunked_create_trailers(ngx_htt
>  b->memory = 1;
>  b->last_buf = 1;
>
> +if (len == sizeof(CRLF "0" CRLF CRLF) - 1) {
> +b->pos = (u_char *) CRLF "0" CRLF CRLF;
> +b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1;
> +return cl;
> +}

Sounds good, but the if statement reads a bit weird.

What about this instead, even though it might be a bit more expensive?

@@ -236,7 +236,7 @@ ngx_http_chunked_create_trailers(ngx_http_request_t *r,
 ngx_list_part_t  *part;
 ngx_table_elt_t  *header;

-len = sizeof(CRLF "0" CRLF CRLF) - 1;
+len = 0;

 part = >headers_out.trailers.part;
 header = part->elts;
@@ -273,12 +273,14 @@ ngx_http_chunked_create_trailers(ngx_http_request_t *r,
 b->memory = 1;
 b->last_buf = 1;

-if (len == sizeof(CRLF "0" CRLF CRLF) - 1) {
+if (len == 0) {
 b->pos = (u_char *) CRLF "0" CRLF CRLF;
 b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1;
 return cl;
 }

+len += sizeof(CRLF "0" CRLF CRLF) - 1;
+
 b->pos = ngx_palloc(r->pool, len);
 if (b->pos == NULL) {
 return NULL;

Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 1 of 3] Added support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Fri, Jun 02, 2017 at 08:33:45PM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora 
> # Date 1490351854 25200
> #  Fri Mar 24 03:37:34 2017 -0700
> # Node ID 41c09a2fd90410e25ad8515793bd48028001c954
> # Parent  716852cce9136d977b81a2d1b8b6f9fbca0dce49
> Added support for trailers in HTTP responses.
> 
> Example:
> 
>ngx_table_elt_t  *h;
> 
>h = ngx_list_push(>headers_out.trailers);
>if (h == NULL) {
>return NGX_ERROR;
>}
> 
>ngx_str_set(>key, "Fun");
>ngx_str_set(>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.
> 
> Modules that want to emit trailers must set r->expect_trailers = 1
> in header filter, otherwise they might not be emitted for HTTP/1.1
> responses that aren't already chunked.
> 
> This change also adds $sent_trailer_* variables.
> 
> Signed-off-by: Piotr Sikora 

Overall looks good, see some additional comments below.

> 
> diff -r 716852cce913 -r 41c09a2fd904 
> 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,7 @@ 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);
>  
>  
>  static ngx_http_module_t  ngx_http_chunked_filter_module_ctx = {
> @@ -69,27 +70,28 @@ ngx_http_chunked_header_filter(ngx_http_
>  return ngx_http_next_header_filter(r);
>  }
>  
> -if (r->headers_out.content_length_n == -1) {
> -if (r->http_version < NGX_HTTP_VERSION_11) {
> +if (r->headers_out.content_length_n == -1 || r->expect_trailers) {
> +

I actually think that using two lines as initially suggested is 
more readable and more in line with current style.  YMMV.

-if (r->headers_out.content_length_n == -1 || r->expect_trailers) {
-
+if (r->headers_out.content_length_n == -1
+|| r->expect_trailers)
+{

[...]

> @@ -230,6 +223,105 @@ ngx_http_chunked_body_filter(ngx_http_re
>  }
>  
>  
> +static ngx_chain_t *
> +ngx_http_chunked_create_trailers(ngx_http_request_t *r)
> +{

[...]

> +len += header[i].key.len + sizeof(": ") - 1
> +   + header[i].value.len + sizeof(CRLF) - 1;
> +}
> +
> +ctx = ngx_http_get_module_ctx(r, ngx_http_chunked_filter_module);

Usual approach is to pass context into internal functions if 
needed and already available in the calling functions.

 static ngx_chain_t *
-ngx_http_chunked_create_trailers(ngx_http_request_t *r)
+ngx_http_chunked_create_trailers(ngx_http_request_t *r,
+ngx_http_chunked_filter_ctx_t *ctx)
 {

(and more, see full patch below)

> +
> +cl = ngx_chain_get_free_buf(r->pool, >free);
> +if (cl == NULL) {
> +return NULL;
> +}
> +
> +b = cl->buf;
> +
> +b->tag = (ngx_buf_tag_t) _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;
> +}

I would prefer to preserve the typical code path (when there are no 
trailers) without an extra allocation.  It looks like it would be 
as trivail as:

@@ -273,14 +273,18 @@ ngx_http_chunked_create_trailers(ngx_htt
 b->memory = 1;
 b->last_buf = 1;
 
+if (len == sizeof(CRLF "0" CRLF CRLF) - 1) {
+b->pos = (u_char *) CRLF "0" CRLF CRLF;
+b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1;
+return cl;
+}
+
 b->start = ngx_palloc(r->pool, len);
 if (b->start == NULL) {
 return NULL;
 }

Note well that b->start is intentionally not touched in the 
previous code.  As buffers are reused, b->start, if set, is 
expected to point to a chunk of memory big enough to contain 
"" CRLF, as allocated in the 
ngx_http_chunked_body_filter().

While this is not critical in this particular code path, as 
last-chunk is expected to be only created once, the resulting code 
is confusing: while it provides b->tag to make the buffer 
reusable, it doesn't maintain required invariant on b->start.

Trivial solution would be to avoid setting b->start / b->end as it 
was done in the previous code and still done in the CRLF case.

-b->start = ngx_palloc(r->pool, len);
-if (b->start == NULL) {
+b->pos = ngx_palloc(r->pool, len);
+if (b->pos == NULL) {
 return NULL;
 }
 
-b->end = b->last + len;
-b->pos = b->start;
-b->last = b->start;
+b->last = b->pos;


Full patch with the above comments:

diff --git a/src/http/modules/ngx_http_chunked_filter_module.c 
b/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
@@ 

Re: [PATCH 1 of 3] Added support for trailers in HTTP responses

[Piotr Sikora via nginx-devel]

Hey Maxim,

> Note: the "TE: trailers" requirement is no longer present in the
> code.

Good catch, thanks!

> 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).

Oops, this regression is a result of removal of r->accept_trailers,
which previously disallowed trailers in HTTP/1.0 requests.

> 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;
> }
> }

Applied with small style changes.

> 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.

Done.

> There is no need to write sizeof() so many times, just
>
> len += sizeof(CRLF "0" CRLF CRLF) - 1;
>
> would be enough.

Done.

Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 1 of 3] Added support for trailers in HTTP responses

[Piotr Sikora via nginx-devel]

# HG changeset patch
# User Piotr Sikora 
# Date 1490351854 25200
#  Fri Mar 24 03:37:34 2017 -0700
# Node ID 41c09a2fd90410e25ad8515793bd48028001c954
# Parent  716852cce9136d977b81a2d1b8b6f9fbca0dce49
Added support for trailers in HTTP responses.

Example:

   ngx_table_elt_t  *h;

   h = ngx_list_push(>headers_out.trailers);
   if (h == NULL) {
   return NGX_ERROR;
   }

   ngx_str_set(>key, "Fun");
   ngx_str_set(>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.

Modules that want to emit trailers must set r->expect_trailers = 1
in header filter, otherwise they might not be emitted for HTTP/1.1
responses that aren't already chunked.

This change also adds $sent_trailer_* variables.

Signed-off-by: Piotr Sikora 

diff -r 716852cce913 -r 41c09a2fd904 
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,7 @@ 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);
 
 
 static ngx_http_module_t  ngx_http_chunked_filter_module_ctx = {
@@ -69,27 +70,28 @@ ngx_http_chunked_header_filter(ngx_http_
 return ngx_http_next_header_filter(r);
 }
 
-if (r->headers_out.content_length_n == -1) {
-if (r->http_version < NGX_HTTP_VERSION_11) {
+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 {
-clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
-
-if (clcf->chunked_transfer_encoding) {
-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 {
-r->keepalive = 0;
-}
 }
 }
 
@@ -179,26 +181,17 @@ ngx_http_chunked_body_filter(ngx_http_re
 }
 
 if (cl->buf->last_buf) {
-tl = ngx_chain_get_free_buf(r->pool, >free);
+tl = ngx_http_chunked_create_trailers(r);
 if (tl == NULL) {
 return NGX_ERROR;
 }
 
-b = tl->buf;
-
-b->tag = (ngx_buf_tag_t) _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;
 
 *ll = tl;
 
 if (size == 0) {
-b->pos += 2;
+tl->buf->pos += 2;
 }
 
 } else if (size > 0) {
@@ -230,6 +223,105 @@ ngx_http_chunked_body_filter(ngx_http_re
 }
 
 
+static ngx_chain_t *
+ngx_http_chunked_create_trailers(ngx_http_request_t *r)
+{
+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 = sizeof(CRLF "0" CRLF CRLF) - 1;
+
+part = >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;
+}
+
+ctx = ngx_http_get_module_ctx(r, ngx_http_chunked_filter_module);
+
+cl = ngx_chain_get_free_buf(r->pool, >free);
+if (cl == NULL) {
+return NULL;
+}
+
+b = cl->buf;
+
+b->tag = (ngx_buf_tag_t) _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 

Re: [PATCH 1 of 3] Added support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Fri, Jun 02, 2017 at 02:04:06AM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora 
> # 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(>headers_out.trailers);
>if (h == NULL) {
>return NGX_ERROR;
>}
> 
>ngx_str_set(>key, "Fun");
>ngx_str_set(>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 
> 
> 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, >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, >free);
> +if (tl == NULL) {
> +return 

[PATCH 1 of 3] Added support for trailers in HTTP responses

[Piotr Sikora via nginx-devel]

# HG changeset patch
# User Piotr Sikora 
# 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(>headers_out.trailers);
   if (h == NULL) {
   return NGX_ERROR;
   }

   ngx_str_set(>key, "Fun");
   ngx_str_set(>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).

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 

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;
+
+} 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, >free);
-if (tl == NULL) {
-return NGX_ERROR;
+
+if (r->expect_trailers) {
+tl = ngx_http_chunked_create_trailers(r, size ? 1 : 0);
+
+} else {
+tl = NULL;
 }
 
-b = tl->buf;
+if (tl == NULL) {
+tl = ngx_chain_get_free_buf(r->pool, >free);
+if (tl == NULL) {
+return NGX_ERROR;
+}
 
-b->tag = (ngx_buf_tag_t) _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) _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, >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 = >headers_out.trailers.part;
+header = part->elts;
+
+for (i = 0; /* void */; i++) {
+
+if (i >= part->nelts) {
+if (part->next == NULL) {
+break;
+}
+
+