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