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

Move early hints into responseEarlyHints field #524

Merged
merged 12 commits into from
Dec 31, 2023

Conversation

thomasjm
Copy link
Contributor

This is a continuation of #523, developing the idea that we can put the early hints into a new field of the Response object called responseEarlyHints.

AFAICT this is not a breaking change, since the Response data constructors are not exposed and the StatusHeaders type is only exposed in an internal module.

@thomasjm
Copy link
Contributor Author

Hmm, I just realized this is not going to actually provide a performance improvement, because parseStatusHeaders won't return until it actually reads the final status + headers, at which time the response is presumably ready to go anyway.

This limits the usefulness of this when it comes to implementing performant reverse proxying, but at least it fixes the behavior on #521.

It seems like it will require more of an API change in order to first return early hints headers, and then at a later time return the actual status + headers + response.

@thomasjm
Copy link
Contributor Author

I just added the cabal version bump and changelog update.

I'm still not sure how the early hint headers can be exposed in a way that the user can access them immediately after they arrive, as when you're trying to e.g. write a reverse proxy. When you look at the key functions of http-client such as responseOpen:

responseOpen :: Request -> Manager -> IO (Response BodyReader)

it seems like it would have to change to something like this:

-- An extra callback that receives any early hint headers
responseOpen' :: Request -> Manager -> ([Header] -> IO ()) -> IO (Response BodyReader)

OR, less invasively, how about being able to specify a [Header] -> IO () callback on the Request object? It already has a couple IO callbacks.

@snoyberg
Copy link
Owner

I think adding a callback to the Request object makes perfect sense in a case like this. Do you want to include that in this PR before I merge?

@thomasjm
Copy link
Contributor Author

I might as well include it, it looks like I'll have to plumb the callback through most of this.

@thomasjm
Copy link
Contributor Author

Okay, I've added the callback.

@thomasjm
Copy link
Contributor Author

thomasjm commented Dec 29, 2023

Okay, the callback signature is [Header] -> IO () now and I added a test of multiple 103 header sections.

@snoyberg snoyberg merged commit a1c5e34 into snoyberg:master Dec 31, 2023
12 of 15 checks passed
@snoyberg
Copy link
Owner

Merged and released, thanks!

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

Successfully merging this pull request may close these issues.

2 participants