-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
Support SendResponse::send_continue #601
base: master
Are you sure you want to change the base?
Conversation
One possible alternate solution, could be to implement more of the functionality directly, by hooking up the first attempt to read the request-body, as a signal to send |
@@ -111,7 +111,7 @@ impl State { | |||
Open { local, remote } | |||
} | |||
} | |||
HalfClosedRemote(AwaitingHeaders) | ReservedLocal => { | |||
HalfClosedRemote(AwaitingHeaders | Streaming) | ReservedLocal => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some time thinking about this in particular. There is no documentation on the struct hinting at how to reason about the state, but if I understood it correctly HalfClosedRemote
means that we've gotten an EOS from the remote side. HalfClosedRemote(AwaitingHeaders)
this side currently haven't written headers yet (the enum-name Peer
is a bit misleading here?). This used to encode the rule that headers should only be sent once. With 100-continue in the picture, however, that doesn't really hold true. The easiest way I could see to model this, is to simply relax this rule as proposed here. Perhaps another approach could be feasible? I.E. something not "Opening the send-half", but instead explicitly "finish sending headers"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code comments at the top of file include the flow chart from the spec, but the the spec includes more description of each possible state: https://httpwg.org/specs/rfc7540.html#StreamStates.
It looks like your understanding is correct. I've also found the usage of Peer
in this crate to be the opposite of what I usually think, but it is consistently used in this file and elsewhere to mean "me" (I usually think of peer as the remote, but I didn't come up with those names 🤷).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I think we don't want to change the state here, since sending a HEADERS
frame with a 1xx code shouldn't mean it is now waiting to send DATA
frames. It still needs to send the "final" code in another HEADERS
frame.
@@ -111,7 +111,7 @@ impl State { | |||
Open { local, remote } | |||
} | |||
} | |||
HalfClosedRemote(AwaitingHeaders) | ReservedLocal => { | |||
HalfClosedRemote(AwaitingHeaders | Streaming) | ReservedLocal => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code comments at the top of file include the flow chart from the spec, but the the spec includes more description of each possible state: https://httpwg.org/specs/rfc7540.html#StreamStates.
It looks like your understanding is correct. I've also found the usage of Peer
in this crate to be the opposite of what I usually think, but it is consistently used in this file and elsewhere to mean "me" (I usually think of peer as the remote, but I didn't come up with those names 🤷).
src/server.rs
Outdated
/// [`SendResponse`]: # | ||
/// [`send_reset`]: #method.send_reset | ||
/// [`send_response`]: #method.send_response | ||
pub fn send_continue(&mut self) -> Result<(), crate::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be out-of-scope to suggest this, but should we perhaps consider a slightly broader method, that allows sending other informational responses, like 103 Early Hints
or future codes? In that case, we'd want to be able to send headers too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it when writing the PR, but avoided it since I'm not sure what it's signature should be. send_info(Response<()>)
feels a bit too wide, given that practically only 1xx responses are allowed here. OTOH, send_response(Response<()>)
is also wider than it technically supports. (Extensions are cleared, the body unused, and the only Version
supported really, is HTTP2), so maybe such API is fine.
@@ -111,7 +111,7 @@ impl State { | |||
Open { local, remote } | |||
} | |||
} | |||
HalfClosedRemote(AwaitingHeaders) | ReservedLocal => { | |||
HalfClosedRemote(AwaitingHeaders | Streaming) | ReservedLocal => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I think we don't want to change the state here, since sending a HEADERS
frame with a 1xx code shouldn't mean it is now waiting to send DATA
frames. It still needs to send the "final" code in another HEADERS
frame.
Thank you 🙏 for a good and clear review. I've changed the according to the feedback. |
Ping |
Ping |
Ping @seanmonstar |
Maybe @LucioFranco have some time for a review? 🙏 |
Any progress? It's an important feature |
@seanmonstar This is ready for a new review, do you have anytime to do it? |
Any update on this @seanmonstar @LucioFranco ? |
As discussed on discord, I'm willing to help out in any capacity that you can use me to get this PR shipped. Be it more code contribution, review, testing, docs, ... do let me know. |
/// [`SendResponse`]: # | ||
/// [`send_reset`]: #method.send_reset | ||
/// [`send_response`]: #method.send_response | ||
pub fn send_info(&mut self, response: Response<()>) -> Result<(), crate::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this just needs to document with a # Panics
section that passing a response with a non-1xx status code is a misuse of the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and fixed.
Support for `Expect: 100-continue` is mandated as MUST by https://datatracker.ietf.org/doc/html/rfc7231#section-5.1.1. Yet servers built on `h2` cannot currently support this requirement. One example of such usage, is [hyper #2743](hyperium/hyper#2743). This approach adds a `send_info` method to `SendResponse` that a server application can use to implement support itself. This PR does _not_ solve the feature itself, it merely provides sufficient support for a server application to implement the functionality as desired.
Support for
Expect: 100-continue
is mandated as MUST byhttps://datatracker.ietf.org/doc/html/rfc7231#section-5.1.1. Yet servers
built on
h2
cannot currently support this requirement.One example of such usage, is hyper #2743.
This approach adds a
send_continue
method toSendResponse
that a serverapplication can use to implement support itself. This PR does not solve
the feature itself, it merely provides sufficient support for a server
application to implement the functionality as desired.