Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
satom99 opened this issue Nov 7, 2022 · 9 comments
Closed

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

satom99 opened this issue Nov 7, 2022 · 9 comments

Comments

@satom99
Copy link

satom99 commented Nov 7, 2022

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
Copy link
Member

Gazler commented Nov 8, 2022

This was first mentioned in #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
Copy link
Contributor

mtrudel commented Nov 8, 2022

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:

  • Define Plug.Conn.Adapter.complete_chunked/? callback (I'm unsure of arity)
  • Define Plug.Conn.complete_chunked/2 function in terms of the adapter
  • Implement this on top of Cowboy (should be straightforward)
  • Implement trailer sending in Bandit for HTTP/1 & HTTP/2. This should also be straightforward, though it will be more involved than Cowboy since the support isn't there already

@josevalim
Copy link
Member

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
Copy link
Author

satom99 commented Mar 17, 2023

@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
Copy link
Contributor

mtrudel commented Mar 17, 2023

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
Copy link
Member

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

@mtrudel
Copy link
Contributor

mtrudel commented Mar 17, 2023

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
Copy link
Contributor

mtrudel commented Mar 17, 2023

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

@josevalim
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants