elixir-plug / plug

Compose web applications with functions
https://hex.pm/packages/plug
Other
2.84k stars 582 forks source link

Need of a Plug.Conn.send_trailers/2 function #1121

Closed satom99 closed 1 year ago

satom99 commented 1 year ago

Hello 👋 I see there is no function in Plug.Conn that allows sending http trailers at the moment.

To circumvent this (because I am using the plug_cowboy adapter) I am currently calling :cowboy_req.stream_trailers/2 manually by grabbing the cowboy request from the conn.adapter field.

I was wondering what would take to (perhaps) introduce a Plug.Conn.send_trailers/2 function and what implications this would have over the existing adapters for Plug - since we would be adding a new required callback.

I am open to work on a PR (or multiple) but I wanted to discuss the matter first.

Thank you!

Gazler commented 1 year ago

This was first mentioned in https://github.com/elixir-plug/plug/pull/535

I believe trailers can be sent in chunked responses for HTTP/1.1 (https://www.rfc-editor.org/rfc/rfc7230#section-4.1.2) These are also supported in HTTP/2 https://www.rfc-editor.org/rfc/rfc7540#section-8.1

I'm not sure what the API should look like if we support this. Given that it is only applicable to streaming responses, perhaps something like the proposed complete_chunked/2 from the issue would work. Maybe a more explicit function name like chunk_trailers/2?

@mtrudel how does this apply to Bandit?

mtrudel commented 1 year ago

Bandit doesn't support sending headers today (though it supports receiving them, and just throws them on the floor). However, I've always planned on revisiting the case in #535 in the future, and so Bandit has been designed to ensure that adding trailer support in the future is near trivial. Cowboy also supports trailers today, so it would be a straightforward addition to plug.

I agree that complete_chunked/2 is probably the best home for this; it also ties off a loose end on the chunked encoding API. I'm not fussed about the name, but I do think it should semantically the same API call that closes off a chunked response.

TBH I'm surprised that a need for this has never come up before, given Elixir's applicability in streaming scenarios where trailers are useful (one of the few places they are!). I think this is probably something worth pursuing, though I wasn't personally planning on tackling it until after Bandit 0.8 (likely early 2023).

A rough approach would be:

josevalim commented 1 year ago

Do you have examples of where trailers are useful? Last time I checked they were not really recommended for HTTP clients because not all proxies implemented them properly which led to them being dropped.

satom99 commented 1 year ago

@josevalim An example for trailers would be gRPC. In the official HTTP2 specification for gRPC you can see that response trailers are used to signal the response status code and error reason when applicable — see the Responses section.

However, in that same specification in the Requests section they mention a special request header:

  • TE → "te" "trailers" # Used to detect incompatible proxies

Which, according to the following message from the gRPC mailing list:

Eric Anderson Jul 13, 2019, 12:56:34 AM
See https://tools.ietf.org/html/rfc7230#section-4.3
Proxy would mainly be an L7 reverse proxy, like, say Nginx. When HTTP reverse proxies don't support trailers, they are supposed to remove "TE: trailers" from the request. If "TE: trailers" makes it from the client to the server it provides stronger confidence that trailers sent from the server to the client will actually arrive. gRPC depends on the trailers, and doesn't operate when they are dropped. See also the last paragraph of section 4.1.2: https://tools.ietf.org/html/rfc7230#section-4.1.2

means that such a header is used to indicate whether all downstream clients accept trailers. So then an incompatible proxy would remove the header to indicate it does not support trailers. And whenever such an "incompatible" request is received by the gRPC server, it must be rejected because the protocol relies on HTTP 2.0 and requires response trailers.

(Side note: the elixir-grpc library calls :cowby_req.stream_trailers/2 the same way I explained above — see here.)

I should be able to get back to this and implement the suggested functionality in the upcoming weeks. Thank you all for taking the time to look into this. 🙂

mtrudel commented 1 year ago

This work never came up from any personal need (though I've used them once in the past and they've always been a nice little bit of obscure trivia I've held on to). IIRC I'd stumbled over it when implementing Bandit's HTTP/2 support, and in furtherance of Bandit's goal to codify as little policy as possible I'd wanted to see if there was an appetite to bring them into the Plug API.

Honestly though, it does seem like a curious omission, given that the places where trailers are useful (providing trailing checksums and other such 'after the fact' data to streaming (usually chunked) responses) are exactly the sorts of places where Elixir's strengths are most apparent.

I still think the approach I suggest above stands, and I'm happy to work it up 'soon' (sometime after April, as I've got my hands full with Bandit work until ElixirConf). It would both tie off @satom99's ask, and also the naggling inconsistency in the chunked API I'd noted in #535.

If there's consensus for this (and nobody else wants to pick it up) I'm happy to schedule the work right after ElixirConf EU.

josevalim commented 1 year ago

Could Plug realistically be used for gRPC if we add trailing commas?

mtrudel commented 1 year ago

Could Plug realistically be used for gRPC if we add trailing commas?

Not in a very comprehensive or ergonomic sense, no. There has been some previous discussion of this here and here. The short version of it is that a comprehensive implementation requires more of a rich lifecycle than an in-process c:Plug.call/2 call. It's come up quite a bit over the past year though; may be worth exploring how to extend Plug (or maybe use our new upgrade support to punt to a new library).

mtrudel commented 1 year ago

With trailer support you could kinda-sorta do part of the spec with Plug as it exists today, but it's pretty awkward.

josevalim commented 1 year ago

Alright, so I think I would wait for a use case outside of gRPC, given that is not suitable for Plug anyway. Again, I am fine with having the feature, but I haven't heard about use cases for Plug yet. :) Thank you!