-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
Add server support for push #327
Conversation
The test I added doesn't pass.
in
So we write When what we really want is Obvious option would I guess be to only call |
@michaelbeaumont thanks for doing this <3 I just got home after traveling. I'm going to review this in more depth shortly. Regarding the issue you described, it sounds tricky indeed. The required behavior is to not send any frames for a push promise stream until the push promise is sent. It sounds like this behavior needs to be coded in. One option would be to not schedule a push promise stream for send if the push promise frame has not been written. This would be tracked in the stream state. So, if the headers frame is pushed, but the push promise was not written, then the stream is not scheduled for send. Once the push promise frame is written, at that point, the stream's pendign frames is checked to see if there is anything. If there is a frame, then the stream is scheduled for send at that point. Thoughts? |
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.
Great start 👍 I left some comments inline.
src/server.rs
Outdated
pub fn push_request( | ||
&mut self, | ||
request: Request<()>, | ||
) -> Result<SendResponse<B>, ::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.
Since this returns a SendResponse
, it looks like we will have a similar problem as with the client half.
Only "root" streams may have push promises. Calling push_request
on the returned SendResponse
should fail. Is there a test for this?
Or should this function return SendPushedResponse
, a type similar to SendResponse
but without a push_request
function?
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.
My plan was definitely the SendPushedResponse
! I think that's the cleanest option.
@@ -283,9 +283,86 @@ impl fmt::Debug for Headers { | |||
} | |||
} | |||
|
|||
// ===== util ===== | |||
|
|||
pub fn parse_u64(src: &[u8]) -> Result<u64, ()> { |
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.
Since this is used in multiple locations now (I believe), I would consider creating a private util
module at the root and moving this there.
src/frame/headers.rs
Outdated
} | ||
} | ||
|
||
pub fn validate_request(req: &Request<()>) -> bool { |
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.
This cleanup work is good standalone. It might be worth extracting it to a dedicated PR in order to simplify this PR (and make reviewing easier). Up to you though! Whatever makes it easier for you.
Unrelated, I'm thinking that, when writing frames, |
I've actually fixed the issue I mentioned in my first comment, in the second commit. By pushing the But then there's unexpected behavior, as evidened by the test (which actually runs successfully, not sure why travis didn't build that commit). Although we send the headers for
Exactly, I think this is the crux of the issue. This seems the only way to enforce the ordering of frames like this and would fix the issue with the 2nd commit. |
It seems to me that it would be possible for |
True, not a good solution. There will need to be backpressure support for pushed requests similar to |
@carllerche do you have time to take another look? |
I need to use HTTP/2 server push. How much of the work is done for a bare minimum functional server push? Is resolving the conflicts here still applicable? |
Any progress on this? I also needed the HTTP/2 server_push extension. What's good for an HTTP/2 without support for the server_push extension. |
Tests still need to be rewritten, I'll work on it |
This comment has been minimized.
This comment has been minimized.
It's kinda mean to say that this is useless without this option... |
We haven't abandoned the library. We've just so far not needed server push, and so haven't spent time working on it. However, Michael has done some excellent work here, hopefully we can get this functionality in soon! |
use http::request::Parts; | ||
|
||
if let Err(_) = frame::PushPromise::validate_request(&request) { | ||
return Err(UserError::MalformedHeaders); |
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 we should log the PushPromiseError, like at debug level.
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.
Looks good to me, thanks!
Looks like there's some merge conflicts? |
@seanmonstar The branch is up to date as far as I can tell |
itshappening.gif |
Wow amazing job guys! Congrats, you are the first Rust crate to handle HTTP2 Server push! |
Thank you all for the hardwork. |
hi @michaelbeaumont , there is a posted example/issue on how to use h2 to do server_push. I have the same problem, can you pinpoint what the code does wrong here? |
Closes #291, closes #185