Add HTTP Trailers extension #339

Kludex · 2022-08-14T18:50:28Z

Closes Protocol: Support for HTTP trailers #141

Currently, there are two implementations of HTTP Trailer:

Add support for Trailers encode/uvicorn#1599 (Uvicorn)
https://github.com/nonebot/nonecorn/blob/nonecorn_dev/src/hypercorn/protocol/http_stream.py#L232 (Nonecorn - Hypercorn fork) - Commit that introduced it: nonebot/nonecorn@222757f

What this PR proposes doesn't match any of those two implementations. I've tried to simplify a bit...

The uvicorn one introduces a new key on the http.response.start called trailers, to show the intention from the application to the server that trailing headers will be sent. I think this is not needed. The server can understand the intention if the header Trailer: is provided on the headers from the http.response.start.

The nonecorn uses a different name for the extension, but it was the first one implemented. I just thought the name http.response.trailers was a bit better, but happy to change if wanted. cc @synodriver

I understood less about the nonecorn implementation in comparison to the one from uvicorn, so I may be missing something here that I couldn't grasp from there. So I'd like your opinion here @synodriver. 🙏

The nonecorn implementation makes the ASGI framework in charge of sending the Trailer: header, and it sets the more_body field to True on the last http.response.body message, and then it's able to use trailing headers message (called http.trailingheaders.send there) as last message, and then close the transport after it.

cc @simonw @abersheeran @pgjones @synodriver @sam-kleiner @tomchristie

References used to write this:

docs/extensions.rst

Kludex

@synodriver According to what we talked, is this what you think it should be written?

docs/extensions.rst

tomchristie · 2022-08-17T10:37:52Z

(Because I've been cc'ed)

Does it matter if we've seen enough of a genuine use case or not? I'm aware that HTTP trailing headers exist in the spec, but I've never encountered their usage in my career. So I'd be a bit wary of introducing that complexity into the spec, and the expectation that can then introduce onto other projects to include that complexity in their implementations.

I'd probably put an "HTTP trailing headers" extension into the "technology for the sake of technology", unless there was a clearer indication that it's solving someone's actual use-case.

Perhaps that'd involve something like a "here's a functional gRPC implementation that we can show works if we have this extension and doesn't work without it" (?)

But, also shrug. 🤷‍♂️

"Sensible pushback" or "Mildly grumpy old geezer"? You decide. 😇

sam-kleiner · 2022-08-17T14:06:13Z

(Because I've been cc'ed)

Does it matter if we've seen enough of a genuine use case or not? I'm aware that HTTP trailing headers exist in the spec, but I've never encountered their usage in my career. So I'd be a bit wary of introducing that complexity into the spec, and the expectation that can then introduce onto other projects to include that complexity in their implementations.

I'd probably put an "HTTP trailing headers" extension into the "technology for the sake of technology", unless there was a clearer indication that it's solving someone's actual use-case.

Perhaps that'd involve something like a "here's a functional gRPC implementation that we can show works if we have this extension and doesn't work without it" (?)

But, also shrug. 🤷‍♂️

"Sensible pushback" or "Mildly grumpy old geezer"? You decide. 😇

I did the uvicorn PR and didnt realize this would be controversial.

I have an actual need for this and some use cases are laid out in that PR. But in short, It is useful when streaming a response in a couple cases. You may encounter an error that you want the client to know about, As you have already responded 200 and sent headers, how else could you get this information to them? You may also be streaming a large file that you want to compute the hash of as it stream through the sever and send that hash at the end.

sam-kleiner · 2022-08-17T14:27:19Z

Copying some comments from my separate PR above.

I wanted to open this to add options to the discussion. I did the uvicorn implementation.

My original POC did not account for chunked trailers but this would be easy to add with a more_trailers property similar to the more_body property. Though this could be an issue in h11 as mentioned above

Actually when you are using h11, there is only one chance to send trailer

The original purpose of the new trailers bool property in http.response.start was to know which args to send to h11.

Personally I like the more_body being set to false when the body is done and trailers being sent after. It makes more sense to me, but at the end of the day if any trailers get added I will be happy.

andrewgodwin · 2022-08-20T22:54:49Z

I think I slightly prefer the design proposed in #341 - it's more explicit, and doesn't rely on parsing the content of headers sent back. Is there any strong reason not to go with that design? (I would want to add the extension signifier over there too, rather than just have it appear in the spec and force a new version)

Kludex · 2022-08-21T05:26:04Z

So, by slightly prefer you mean the trailers field being added to the http.response.start instead of the server inferring? Or is there another difference?

I'm fine with this. The client will still should to send what trailing headers are those, but it's not mandatory.

Is the more_trailers field needed?

andrewgodwin · 2022-08-22T14:49:22Z

Yes, to be specific:

Trailers field in the start message is better than inferring
more_trailers should be there in case the server is chunking them (because if someone is already using trailers, the chances they're doing strange things is much higher)

Kludex · 2022-08-23T19:51:37Z

Is there a scenario on which we have this sequence:

http.response.start
http.response.trailers

Without the http.response.body?

Kludex · 2022-08-23T20:11:14Z

I used the name headers instead of trailers, as there's already a trailers field being introduced in Response Start, and all the headers field have the same type... But I can change that if wanted.

andrewgodwin

I think I'm OK with it being called headers in the trailer response - that is, after all, what they are, rather than trailing body.

andrewgodwin · 2022-08-25T10:50:59Z

asgiref/typing.py

@@ -99,6 +100,7 @@ class HTTPResponseStartEvent(TypedDict):
    type: Literal["http.response.start"]
    status: int
    headers: Iterable[Tuple[bytes, bytes]]
+    trailers: bool


TypedDict is total by default, so this would require trailers to always be present which it won't be (as we're doing this as a non-major change by making it optional). We need to switch this so that trailers is optional, which may need to be done with two inheriting types due to TypedDict not having fine-grained options.

more_body is bool on HTTPResponseBodyEvent. What's the difference between those?

We need to switch this so that trailers is optional, which may need to be done with two inheriting types due to TypedDict not having fine-grained options.

We can use https://peps.python.org/pep-0655/#specification

What both are suggesting was already discussed when the asgiref.typing module was implemented. See #221 (comment).

It was, but that was "types for the first time" whereas this is modifying existing types - if we land this change, anything that currently (correctly) typechecks as a response no longer will, and that seems bad.

The suggestion in that thread of Unioning two other dict types together would also work, though.

It was, but that was "types for the first time"

What's the difference? asgiref is versioned... 🤔

if we land this change, anything that currently (correctly) typechecks as a response no longer will, and that seems bad.

Not sure if it's important, but... uvicorn is the only ASGI server implementation that uses asgiref.typing... 🤷‍♂️

andrewgodwin

OK, I think this is good enough to go. I'll leave a bit more time for any further objections and then we'll merge it.

Kludex · 2022-09-25T09:32:21Z

Thanks everybody for the comments! 🙏

`asgiref==3.6.0` introduced a new required `trailers` key to the `asgiref.types.HTTPResponseStartEvent` type. As the django/asgiref#339 code review pointed out, this is a breaking change for type checking, but it was added in a minor release, and not mentioned in the changelog. django/asgiref#339 django/asgiref#362

Add HTTP Trailer spec to extensions

945784a

Kludex changed the title ~~Add HTTP Trailer spec to extensions~~ Add HTTP Trailer spec Aug 14, 2022

Kludex changed the title ~~Add HTTP Trailer spec~~ Add HTTP Trailer extension Aug 14, 2022

Kludex changed the title ~~Add HTTP Trailer extension~~ Add HTTP Trailers extension Aug 14, 2022

pgjones reviewed Aug 15, 2022

View reviewed changes

docs/extensions.rst Show resolved Hide resolved

docs/extensions.rst Outdated Show resolved Hide resolved

synodriver reviewed Aug 15, 2022

View reviewed changes

docs/extensions.rst Outdated Show resolved Hide resolved

docs/extensions.rst Show resolved Hide resolved

Kludex commented Aug 16, 2022

View reviewed changes

docs/extensions.rst Outdated Show resolved Hide resolved

docs/extensions.rst Show resolved Hide resolved

Kludex commented Aug 17, 2022

View reviewed changes

docs/extensions.rst Outdated Show resolved Hide resolved

sam-kleiner mentioned this pull request Aug 17, 2022

RE: Add HTTP Trailers #341

Closed

Add more_trailers and trailers fields

65da56a

andrewgodwin reviewed Aug 25, 2022

View reviewed changes

Merge branch 'main' into http-trailing-headers

f270edd

Kludex requested review from andrewgodwin and pgjones and removed request for andrewgodwin August 28, 2022 07:22

andrewgodwin approved these changes Sep 17, 2022

View reviewed changes

andrewgodwin merged commit 3b5aaff into django:main Sep 25, 2022

Kludex deleted the http-trailing-headers branch September 25, 2022 09:31

jluebbe mentioned this pull request Jul 1, 2024

Regular gRPC support public/sonora#22

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTTP Trailers extension #339

Add HTTP Trailers extension #339

Kludex commented Aug 14, 2022 •

edited

Loading

Kludex left a comment

tomchristie commented Aug 17, 2022

sam-kleiner commented Aug 17, 2022 •

edited

Loading

sam-kleiner commented Aug 17, 2022 •

edited

Loading

andrewgodwin commented Aug 20, 2022

Kludex commented Aug 21, 2022 •

edited

Loading

andrewgodwin commented Aug 22, 2022

Kludex commented Aug 23, 2022 •

edited

Loading

Kludex commented Aug 23, 2022

andrewgodwin left a comment

andrewgodwin Aug 25, 2022

Kludex Aug 25, 2022

abersheeran Aug 26, 2022

Kludex Aug 26, 2022

andrewgodwin Aug 29, 2022

Kludex Sep 10, 2022

Kludex Sep 10, 2022 •

edited

Loading

andrewgodwin left a comment

Kludex commented Sep 25, 2022

Add HTTP Trailers extension #339

Add HTTP Trailers extension #339

Conversation

Kludex commented Aug 14, 2022 • edited Loading

Kludex left a comment

Choose a reason for hiding this comment

tomchristie commented Aug 17, 2022

sam-kleiner commented Aug 17, 2022 • edited Loading

sam-kleiner commented Aug 17, 2022 • edited Loading

andrewgodwin commented Aug 20, 2022

Kludex commented Aug 21, 2022 • edited Loading

andrewgodwin commented Aug 22, 2022

Kludex commented Aug 23, 2022 • edited Loading

Kludex commented Aug 23, 2022

andrewgodwin left a comment

Choose a reason for hiding this comment

andrewgodwin Aug 25, 2022

Choose a reason for hiding this comment

Kludex Aug 25, 2022

Choose a reason for hiding this comment

abersheeran Aug 26, 2022

Choose a reason for hiding this comment

Kludex Aug 26, 2022

Choose a reason for hiding this comment

andrewgodwin Aug 29, 2022

Choose a reason for hiding this comment

Kludex Sep 10, 2022

Choose a reason for hiding this comment

Kludex Sep 10, 2022 • edited Loading

Choose a reason for hiding this comment

andrewgodwin left a comment

Choose a reason for hiding this comment

Kludex commented Sep 25, 2022

Kludex commented Aug 14, 2022 •

edited

Loading

sam-kleiner commented Aug 17, 2022 •

edited

Loading

sam-kleiner commented Aug 17, 2022 •

edited

Loading

Kludex commented Aug 21, 2022 •

edited

Loading

Kludex commented Aug 23, 2022 •

edited

Loading

Kludex Sep 10, 2022 •

edited

Loading