[PATCH 1 of 3] HTTP: add support for trailers in HTTP responses

Wed May 3 14:00:18 UTC 2017

Hello!

On Sun, Apr 30, 2017 at 09:37:36PM -0700, Piotr Sikora via nginx-devel wrote:

> Hey Maxim,
> 
> > 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".
> 
> I disagree, if someone adds trailers in their configuration, then they
> are hardly optional, therefore they should be sent in response to
> requests with "TE: trailers".

I don't think that deciding for users is a good idea, especially 
given that the only available case is "when trailers are optional" 
(see above, "TE: trailers" does not guarantee anything) and all 
three use cases listed also assume that trailers are optional.

> > 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.
> 
> That's valid argument.

Good to see you've finally got the idea.

That's what I'm trying to say all the time: if "TE: trailers" 
should be considered or not depends on the use case.  In all use 
cases listed so far it should be ignored.

It is also possible that there are some use cases when trailers 
are to be added only with "TE: trailers".  It should be possible 
to only add trailers for such use cases based on a configure test 
though.  (And I really don't think such use cases are in fact 
possible given the problems outlined above.)

> > 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.
> 
> Yes, of course. What I meant is that if some module produces trailers
> and it's enabled in the configuration, then even if there are only few
> obscure clients that break because of it, the only option will be to
> disable them altogether, instead of only sending them in response to
> requests with "TE: trailers".
> 
> Anyway, I'll just remove that requirement for now, and you can re-add
> it later if/when others complain.

Well, this looks like a problem specific to the particular module 
and/or particular trailer being added.  And certainly re-adding 
won't work, as we've already agreed that at least some use cases 
require trailers regardless of "TE: 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.
> 
> I'm confused, it sounds like you're saying two opposite things...
> 
> Are you saying that r->expect_trailers should be removed and we should
> send trailers only if transfer encoding is already chunked and not
> send trailers otherwise?

No.

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).

> > 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.
> >
> > (...)
> >
> > I certainly against intruduction of such a difference between
> > HTTP/1.1 and HTTP/2 behaviour.
> 
> So what do you suggest we do instead? Merge them with headers? Wasn't
> that your biggest concern from the beginning?

Certainly merging is a bad idea either.  May be the solution would 
be avoid sending trailers for header-only requests consistently 
for both HTTP/1.1 and HTTP/2.

> Also, how would you merge them in upstream? Cacheable responses to
> HEAD requests are "terminated" and headers are sent before upstream
> sends complete response, so trailers would be dropped there (at least
> in the existing code).

Yes, certainly it won't be possible to implement this without 
major changes to the current code.  And, even if properly 
implemented, this will require buffering of the whole backend 
response before nginx will be able to answer to the client, so 
probably this is not something we want to deal with.

Note well that this problem is protocol-independent, so it will 
also affect sending trailers in HTTP/2 for header-only requests as 
currently done in your patch.

Just a side note: another related problem is with trailers 
calculated by body filters, such as Content-SHA256.  There are no 
body filters in case of header-only requests, so such filters 
won't be able calculate anything.

-- 
Maxim Dounin
http://nginx.org/