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
