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

fix(http1): only send 100 Continue if request body is polled #2119

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

seanmonstar
Copy link
Member

Before, if a client request included an Expect: 100-continue header,
the 100 Continue response was sent immediately. However, this is
problematic if the service is going to reply with some 4xx status code
and reject the body.

This change delays the automatic sending of the 100 Continue status
until the service has call poll_data on the request body once.

Closes #838

@seanmonstar seanmonstar requested a review from sfackler January 27, 2020 23:30
@@ -460,19 +469,28 @@ impl From<Cow<'static, str>> for Body {
impl Sender {
/// Check to see if this `Sender` can send more data.
pub fn poll_ready(&mut self, cx: &mut task::Context<'_>) -> Poll<crate::Result<()>> {
match self.abort_tx.poll_canceled(cx) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this was important for detecting when the receiver is dropped while the connection is waiting on the socket.

Copy link
Contributor

@sfackler sfackler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@sfackler
Copy link
Contributor

Does this need to be handled for http2 as well?

@seanmonstar
Copy link
Member Author

Could be an enhancement, but the HTTP2 side doesn't currently have any handling of Expect: 100-continue. I believe sending a RST_STREAM is preferred there, which hyper does handle.

@sfackler
Copy link
Contributor

Do you mean the HTTP2 standard doesn't define 100-continue semantics, or that h2 doesn't deal with it right now? RST_STREAM doesn't quite cover the same use cases, since the client doesn't have a robust way to know if it should send the body or not other than I guess waiting for a little bit to see if it gets a RST.

It seems like some implementations do support it, FWIW. Don't feel super strongly about it though: https://github.com/dotnet/corefx/issues/31312

@seanmonstar
Copy link
Member Author

that h2 doesn't deal with it right now?

This one. I haven't done much looking into what HTTP2 says about expect: 100-continue, I just figured RST_STREAM can serve a similar purpose, and h2 does handle that (a RST_STREAM is sent if the body is dropped).

@seanmonstar seanmonstar force-pushed the expect-continue-on-body-poll branch 2 times, most recently from c0c9fb9 to e06f582 Compare January 29, 2020 01:04
Before, if a client request included an `Expect: 100-continue` header,
the `100 Continue` response was sent immediately. However, this is
problematic if the service is going to reply with some 4xx status code
and reject the body.

This change delays the automatic sending of the `100 Continue` status
until the service has call `poll_data` on the request body once.
@seanmonstar seanmonstar force-pushed the expect-continue-on-body-poll branch from e06f582 to f7c262b Compare January 29, 2020 01:08
@seanmonstar seanmonstar merged commit c4bb4db into master Jan 29, 2020
@seanmonstar seanmonstar deleted the expect-continue-on-body-poll branch January 29, 2020 01:33
@i110
Copy link

i110 commented Nov 17, 2023

Do you have a plan to make the same thing possible with HTTP/2? According to HTTP specifications, an h2 server, like h1, MUST return a 100 continue response to Expect: 100-continue. Also, in practical terms beyond the spec, 100 continue allows for more precise control of body transmission timing than RST_STREAM. I need it now and I'm thinking of creating a patch if there's no ongoing implementation or concerns. If you have any suggestions, please let me know. @seanmonstar

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.

Change 100-continue behavior to send when Body has been polled
3 participants