Hey Maxim,

> Note that we don't use the "HTTP:" prefix.

Maybe it's time to start using it, then? Otherwise, commit messages
are inconsistent.

> Overral, I see at least the following problems with the approach
> taken:
>
> 1. The behaviour depends on the "TE: trailers" header - trailers
> are not sent if the client did not used it.
>
> This is not what HTTP specification says though. Trailers can be
> sent regardless of "TE: trailers" if they are optional. Moreover,
> "TE: trailers" does not guarantee anything either, see
> https://github.com/httpwg/http11bis/issues/18.

From RFC7230, sec. 4.3:

The presence of the keyword "trailers" indicates that the client is
willing to accept trailer fields in a chunked transfer coding, as
defined in Section 4.1.2, on behalf of itself and any downstream
clients.

and sec. 4.1.2:

Unless the request includes a TE header field indicating "trailers"
is acceptable, as described in Section 4.3, a server SHOULD NOT
generate trailer fields that it believes are necessary for the user
agent to receive. Without a TE containing "trailers", the server
ought to assume that the trailer fields might be silently discarded
along the path to the user agent.

Also, in practice, most (all?) clients that consume trailers always
send "TE: trailers" header, so I'm not sure why do you think that's
invalid behavior.

I'm not particularly interested in fighting over this for another X
months, so I'm going to remove this requirement, but for the record, I
think that's a mistake. Especially, since enabling trailers will spam
browsers (which at this point don't care about trailers) with
unnecessary traffic.

Also, repeating myself from last year:

I can drop this requirement if you insist, but that's much less
conservative approach than NGINX usually takes and I expect that some
obscure HTTP clients will break because of lack of proper support for
trailer-part in chunked encoding.

> 2. The code doesn't try to send trailers if r->expect_trailers is
> not set even if we can do so in a particular connection. This
> seems to be completely unneed limitation.

This was added in response to your comments about original patch
forcing chunked encoding, even when trailers were not being emitted.

Using r->expect_trailers allows trailer producers (headers filter,
proxy module, etc.) to indicate that trailers might be produced and
therefore force chunked encoding in HTTP/1.1 requests when needed.

Also, I'm not sure why do you think that's unnecessary limitation,
since r->expect_trailers will be always set if there are trailers.

> 3. The approach with headers and trailers clearly separated and
> never merged has obvious benefits, but it doesn't allow to use trailers in
> header-only responses. This is likely to result in multiple
> problems if we'll try to support anything more than just adding
> trailers for some responses: e.g., caching will certainly loose
> trailers in some cases. The particular patch also creates an
> inconsistency between HTTP/1.1 and HTTP/2 by trying to send
> trailers in HTTP/2 header-only responses. This is likely to
> result in additional problems as well.

What problems, exactly?

> This creates a serious inconsistency between HTTP/1.1 and HTTP/2 by
> sending trailers in header-only responses with HTTP/2, but
> not HTTP/1.1.

There are no means for sending trailers in HTTP/1.1 responses without
a body, and I don't see a reason why we should cripple HTTP/2, simply
because HTTP/1.1 didn't support this.

Let me know if this is a blocker for you or not.

> Logically, trailer headers make no sense in a response without a
> body: if there is no body, there should be no trailer headers, and
> all headers should be normal headers.

I disagree. Trailers are separate from headers and there are valid use
cases (for example, calculating checksum of uploaded object), where
delaying headers until trailer value is calculated could result in
timeouts and/or invalid retries.

> This also brings back the old question of merging trailer headers
> and normal headers. It doesn't seem to be possible to properly
> return trailers via HTTP/1.1 if there are 304 reponses and/or HEAD
> requests without merging them with normal headers. Yet we already
> agreed that merging is bad idea from security point of view.

Just don't send trailers in 304 and/or HEAD requests, when using HTTP/1.1.

> I also not sure how HTTP/2 clients would interpret such "two
> HEADERS frames". While it looks formally permitted by RFC 7540
> (for no aparent reason), I'm pretty sure it will cause various
> problems.

Well, it happens so that I tested "two HEADERS frames" with 304 will
Chrome, Firefox, Safari, nghttp and possibly few more clients, I
forgotten about.

None of them had any issues with such responses, so I'm not sure why
do you think otherwise.

> (It might also be a good idea to keep HTTP/2 changes in a separate
> patch.)

Will do.

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