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

feat(client): support Expect: 100-continue #3472

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iasoon
Copy link

@iasoon iasoon commented Dec 11, 2023

This patch modifies hyper clients to respect the Expect: 100-Continue header.
When the header is present on a request, the client will not send the request body
until the 100 Continue informational response is received from the server.
Related issue: #2565

The implementation follows the existing on_informational callback mechanism,
threading the connection state through to the reponse header parsing.

This seems like the most straightforward modification to get Expect: 100-Continue
support, but happy to discuss alternatives.

This patch modifies hyper client to behave correctly when the `Expect:
100-continue` header is set on a request. The request body will now only
be sent once a 100 Continue response is receved from the server.
@seanmonstar
Copy link
Member

Thanks for the PR and taking this on!

One thing I was wondering about is how would someone choose to wait only a certain amount of time for the server to respond? We could add another timeout into the connection state, but if it's not super hard to do outside, I think that's preferable.

A different way of doing this would be to make the Body be responsible for pausing, and thus it could hold onto a timeout there. Perhaps an extension callback would allow a user to signal a oneshot channel that the 100 response was received.

I'm not sure which is best, at the moment.

@seanmonstar seanmonstar added the S-waiting-on-author Status: waiting on the author to provide more info, or make changes. label Dec 15, 2023
@rfairfax
Copy link

FWIW - I hit this bug recently as well and was just about to file something. Thanks @iasoon for proposing a fix. I work on some storage services where this feature is key to avoiding sending large amounts of data that would otherwise be rejected anyway so this is super high value to my users and my service's scalability. I'm glad to test patches if it helps validate things.

A different way of doing this would be to make the Body be responsible for pausing

I actually started going down this route as it seemed like a way to avoid requiring patching hyper but ended up having to give up as there was no good way to signal the continue came through. However, after thinking about what this would look like my 2c is that the connection is the right place to do this as it's part of the protocol and not a property of the body. If you wire up the continue logic in the body's poll method, for example, then failure to choose the right body implementation that can pause results in not speaking the protocol correctly which seems error prone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: waiting on the author to provide more info, or make changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants