http trailers support

[IgorKarymov]

I decide to start with very basic support and improve it step by step with your comments.
This version tested with few different HTTP/2.0 clients.
My questions:

• according to https://tools.ietf.org/html/rfc7230#section-4.1.2 not all header allowed to be sent in trailers part. Should I check this precondition here https://github.com/IgorKarymov/cowboy/blob/master/src/cowboy_req.erl#L659 ? Somewhere else? Nowhere?
• Is it ok that cowboy_req:stream_trailers will return just "ok" or request also will be usable?
• Before send trailers, I should check that TE: trailers header was sent by client, but this header encoding can be more complicated than just one value (for example TE: trailers, deflate), so should I write simple add hock parser (regexp?) in cowboy_req or it's necessary to modify general headers parser?
• If current approach with sending {headers, Headers} message is ok or I should introduce new message {trailers, Headers} ?
• In HTTP/1.1 we should send Trailer: TrailerHeaders
  in response headers with the list of headers that we want to send in trailers (https://tools.ietf.org/html/rfc7230#section-4.4). How-to better to pass such header and validate this condition? Should I introduce new special value similar to resp_cookie, or just simple composed by the user header is ok?
• What behaviour expecting when one of precondition failed? Should I silently finish the stream?

[essen]

OK I will tentatively answer these questions. Note that I am not 100% sure and might change opinion if I later read something I forgot about right now.

1. I would be OK with a first implementation that does not check what headers are sent in trailers. I am not sure how the check should work (blacklist, probably) or if there would be difference between protocols.

2. stream_trailers can return ok, I don't think we need to update Req for any useful purpose.

3. Definitely not regexes! Never regexes. Ever. Anyway, the parsing has already been implemented: https://github.com/ninenines/cowlib/blob/master/src/cow_http_hd.erl#L2782 so just update cowboy_req:parse_header to call this function, and then just call parse_header directly.

4. We definitely want a message called {trailers, ...}.

5. We probably don't want to send trailers if the user has not send a Trailer header with the response. Doing otherwise would be a breach of the protocol I believe (need to double check). That means we should check for that Trailer header in the stream_reply function. I'm not sure how much we want to check though. Probably just keeping an "has_trailers" state would be enough for a start. Then check "has_trailers" when the user tries to send trailers.

This also means that the check for the TE header should probably be done in stream_reply, and only when the user sets the Trailer header.

6. If there are no trailers to be sent, just terminate the stream by sending a {data, ...} message with fin set.

[IgorKarymov]

So here are some updates to this PR.
I believe that only a few things left:

• support for trailers command in cowboy_http (surprisingly harder than for http 2)
• tests should actually work.

To achieve the second one I will have add trailers support for gun. Probably, should looks something like this:
a50b77b#diff-99ea1c8124d0fa671d5c839e894d28cdR671

[essen]

You probably want to name your test function stream_trailers, "do_trailers" will not be considered as a test (everything do_*) is ignored.

Gun support needs to be added indeed. The message should probably be {gun_trailers, ...} and await return {trailers, ...} like you did.

[IgorKarymov]

Another question. I'm trying to check some preconditions here a50b77b#diff-2db000a0dd01503e2f022387f23413cbR709
But resp_headers is not presented in the request...
What is the way to get access to such information?
Also, checking transfer-encoding for equality to <<"chuncked">> also will be usable, but I didn't find a proper way to do this.

[essen]

resp_headers is for the headers that are preset. It's not all the headers.

And transfer-encoding is only for HTTP/1.1 and should not be used outside the cowboy_http module.

[IgorKarymov]

resp_headers is for the headers that are preset. It's not all the headers.

Could you please clarify, you mean, that user will have to set "trailer" header with set_resp_header manually?
But anyway headers will be removed from the request shortly here https://github.com/IgorKarymov/cowboy/blob/a50b77b0f678e2a7769f2770a6d9e0ecf6420e65/src/cowboy_req.erl#L626 and will newer reach stream_trailers call.

And transfer-encoding is only for HTTP/1.1 and should not be used outside the cowboy_http module.

Honestly, some of this preconditions checks, already protocol version related, so.. should I remove/move all of them?

[essen]

You need a separate value in the map, like "has_trailers", see 5).

For the checks, they can be done at the protocol level and we can keep a value in the map indicating that trailers can be sent.

Both values can probably be merged, maybe trailers => allow | [binary()]. Not defined when trailers are not allowed, allow when they are allowed and before the reply, and the list of headers after the reply after checking the trailers header.

[tony612]

I tried this PR, it just works find :)

[tony612]

sorry, it's a typo - "find" should be "fine" (I have no idea how to modify the review comment)

[tony612]

@essen Any plan to merge this PR?

[essen]

After 2.0 sorry. Priorities...

[tony612]

@IgorKarymov I found we have to buffer the trailers if there're DATA in the local_buffer queue, which is implemented for the flow control recently. I do a simple implementation here tony612@d047a94. You may want to add this or something similar after you rebase master later on.😁

[essen]

FYI I got feedback that this works fine and will look into gRPC compatibility soon after 2.0 gets out at the end of the month.

[IgorKarymov]

Hi, everybody. Unfortunately, it's very small chance that I will be able to complete this PR in an acceptable time frame. Sorry for this. If you interested I have a version of gun with trailers support somewhere (because you need one to run the tests). But I affraid that branches is way to far from master and a lot of things are changed.
Actually, I stuck with adding proper support for trailers for HTTP 1.1 (as mentioned above looks like HTTP2 works fine) because feature is not very wide used, and you have to tinkering too much with specs to create somehow acceptable implementation.

[essen]

I've made a full implementation in 39baed6 for both HTTP/1.1 and HTTP/2. I've made different decisions regarding the sending of the trailers command to the connection process: I send it unconditionally. I made this decision because the logic around whether to include trailers or not is protocol-specific and so fits better in the protocol code. The same reasoning is applied for informational responses or for chunked transfer-encoding in general, the user tells Cowboy what it wants to send, and the protocol sends what it can.

Closing, thanks!