Hello! On Wed, Apr 26, 2017 at 11:35:02AM -0700, Piotr Sikora via nginx-devel wrote:
> > Note that we don't use the "HTTP:" prefix. > > Maybe it's time to start using it, then? Otherwise, commit messages > are inconsistent. We are happy enough with the current practice. > > 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. As you can see from the quote, it talks about not generating "trailer fields that it believes are necessary for the user agent to receive". RFC 2616 is even more clear on this, specifically lists two cases when trailers can be generated, section 3.6.1: A server using chunked transfer-coding in a response MUST NOT use the trailer for any header fields unless at least one of the following is true: a)the request included a TE header field that indicates "trailers" is acceptable in the transfer-coding of the response, as described in section 14.39; or, b)the server is the origin server for the response, the trailer fields consist entirely of optional metadata, and the recipient could use the message (in a manner acceptable to the origin server) without receiving this metadata. In other words, the origin server is willing to accept the possibility that the trailer fields might be silently discarded along the path to the client. That is, there clearly two cases when a server can / should send trailers: - when there is "TE: trailers"; - when trailers are optional. Moreover, given that "TE: trailers" does _not_ guarantee anything either (see link above), the only remaining case seems to be "when trailers are optional". > 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. As per previous discussion, main cases, as listed by you, are: 1. Checksums, like Content-SHA256. 2. Logging, like X-Log-Something for centralized logging on a frontend. 3. Trailing status, like X-Status, to provide additional error information to a frontend. All these uses cases hardly require "TE: trailers", as in all cases information seems optional. Moreover, in cases (2) and (3), backend cannot have "TE: trailers" unless it was already present in the original request from the client. Adding such a header would contradict RFC 7230, which says: 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. For requests from an intermediary, this implies that either: (a) all downstream clients are willing to accept trailer fields in the forwarded response; or, (b) the intermediary will attempt to buffer the response on behalf of downstream recipients. So both cases (2) and (3) generally require that "TE: trailers" should be ignored, or it won't be possible to implement them. > 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. Current nginx behaviour is to don't emit trailers by default, and I don't think we anyhow change this beaviour unless explicitly requested via appropriate configuration directives. This looks conservative enough for me, actually. > > 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. I can see that r->expect_trailers can be useful to indicate that the code wants to use trailers, and would like to force chunked encoding if possible. I don't see why it should be required when encoding is already choosen though. If we know that we can use trailers or don't care, we can just add trailers to the response, and assume they will be sent if it is possible to do so. > > 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? If two identical requests over HTTP/1.1 and HTTP/2 return different results - this looks like a problem for me. Sooner or later this difference will become ciritical in some workload, leading to hard-to-diagnose bugs. > > 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. As per HTTP specification, trailers are expected to be sent as normal headers when there is no body, for both HTTP/1.1 and HTTP/2. And I suspect that the fact that HTTP/2 allows two HEADERS frames without body inbetween is more or less unintentional, too. > Let me know if this is a blocker for you or not. I certainly against intruduction of such a difference between HTTP/1.1 and HTTP/2 behaviour. > > 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. Quoting RFC 7320, section 4.1.2: The trailer fields are identical to header fields, except they are sent in a chunked trailer instead of the message's header section. > > 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. Sure, but this breaks the original idea of trailers, and introduces various cases when they simply don't work. -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel