Hello!

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

> Hey Maxim,
>
> > In your patch, you test r->expect_trailers in two places in
> > chunked filter:
> >
> > 1. when you decide whether to use chunked encoding or not, in
> > ngx_http_chunked_header_filter();
> >
> > 2. when you generate trailer, in ngx_http_chunked_body_filter().
> >
> > I mostly agree with (1) (I would like see it configurable too, but
> > it's probably up to a module which adds trailers to decide, so
> > don't belong to this particular patch), but don't see any reasons
> > for (2).
>
> This is done to avoid unexpected behavior when r->expect_trailers
> isn't set, i.e. if module added trailers, but didn't set
> r->expect_trailers, then without (2) trailers would be emitted only if
> Content-Encoding was already chunked, but not otherwise.
>
> Testing r->expect_trailers in both (1) and (2) ensures that Trailers
> are emitted in consistent manner, without depending on unrelated
> factors, like gzip support by the client, etc.

I see two problems here:

a. There may be use cases when forcing chunked encoding is not
desired, but emitting trailers if it is used still makes sense.

b. Nothing stops modules from changing r->expect_trailers when the
response header was already sent and it is already too late to
switch to chunked transfer encoding. Moreover, this will
naturally happen with any module which is simply following the
requirement to set r->expect_trailers to 1 as in your commit log.

So (a) makes (2) excessively limiting, and (b) makes it useless.

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