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

Piotr Sikora piotrsikora at google.com
Sat Jun 3 03:32:31 UTC 2017


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


More information about the nginx-devel mailing list