Add server support for push #327

michaelbeaumont · 2018-10-21T14:26:28Z

Closes #291, closes #185

michaelbeaumont · 2018-10-21T14:36:50Z

The test I added doesn't pass.
What happens:

Prioritize.queue_frame(PushPromise(1, 2), .. , Stream(1) ..)
  Stream(1).pending_send.push_back(.., PushPromise(1,2))
  Prioritize.schedule_send(Stream(1))
    Prioritize.pending_send.push(Stream(1))
Prioritize.queue_frame(PushPromise(1, 4), .. , Stream(1) ..)
  Stream(1).pending_send.push_back(.., PushPromise(1,4))
  Prioritize.schedule_send(Stream(1))
    Prioritize.pending_send.push(Stream(1))
.queue_frame(Headers(4), .. , Stream(4) ..)
  Stream(4).pending_send.push_back(.., Headers(4))
  Prioritize.schedule_send(Stream(4))
    Prioritize.pending_send.push(Stream(4))

pending_send doesn't add duplicates, so we have:
pending_send: [1,4]

in Prioritize.pop_frame loop:

1: Prioritize.pending_send.pop() -> Stream(1)
      Stream(1).pending_send.pop_front -> PushPromise(1,2)
      !Stream(1).pending_send.is_empty() -> Prioritize.pending_send.push(Stream(1))
2: Prioritize.pending_send.pop() -> Stream(4)
      Stream(4).pending_send.pop_front -> Headers(4)
3: Prioritize.pending_send.pop() -> Stream(1)
      Stream(1).pending_send.pop_front -> PushPromise(1,4)

So we write [PushPromise(1,2), Headers(4), PushPromise(1,4)] to our buffer.

When what we really want is [PushPromise(1,2), PushPromise(1,4), Headers(4)]

Obvious option would I guess be to only call pop from Prioritize.pending_send when there's no more Frames left on Stream.pending_send, otherwise peek. Not sure what that effect that might have though.

carllerche · 2018-10-23T22:43:20Z

@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?

carllerche

Great start 👍 I left some comments inline.

carllerche · 2018-10-24T04:39:54Z

src/server.rs

+    pub fn push_request(
+        &mut self,
+        request: Request<()>,
+    ) -> Result<SendResponse<B>, ::Error> {


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?

My plan was definitely the SendPushedResponse! I think that's the cleanest option.

carllerche · 2018-10-24T04:44:50Z

src/frame/headers.rs

@@ -283,9 +283,86 @@ impl fmt::Debug for Headers {
    }
 }

+// ===== util =====
+
+pub fn parse_u64(src: &[u8]) -> Result<u64, ()> {


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.

carllerche · 2018-10-24T04:45:57Z

src/frame/headers.rs

+        }
+    }
+
+    pub fn validate_request(req: &Request<()>) -> bool {


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.

carllerche · 2018-10-24T04:49:09Z

Unrelated, I'm thinking that, when writing frames, prioritize should keep writing frames from the same stream until after a data frame is reached...

michaelbeaumont · 2018-10-24T08:46:22Z

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.

I've actually fixed the issue I mentioned in my first comment, in the second commit. By pushing the PushPromise(1,4) (which technically belongs to Stream(1)) onto Stream(4)'s pending_send (i.e. before Headers(4)) it's ensured that they don't get sent out of order.

But then there's unexpected behavior, as evidened by the test (which actually runs successfully, not sure why travis didn't build that commit).
https://github.com/carllerche/h2/blob/287c36bb547e20edb2c7f8e760d86956f8827a7e/tests/h2-tests/tests/server.rs#L145-L151

Although we send the headers for Stream(4) "first", the headers for Stream(1) are received first. Definitely not the best ordering for most use cases of push.

https://github.com/carllerche/h2/blob/287c36bb547e20edb2c7f8e760d86956f8827a7e/tests/h2-tests/tests/server.rs#L126-L129

Unrelated, I'm thinking that, when writing frames, prioritize should keep writing frames from the same stream until after a data frame is reached...

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.

carllerche · 2018-10-29T21:08:53Z

I've actually fixed the issue I mentioned in my first comment, in the second commit. By pushing the PushPromise(1,4) (which technically belongs to Stream(1)) onto Stream(4)'s pending_send (i.e. before Headers(4)) it's ensured that they don't get sent out of order.

It seems to me that it would be possible for Stream(1) to be closed before the push promise is sent out? The order in which frames are sent out for any given stream should not be assumed. The prioritization logic is free to change.

This reverts commit 287c36b.

michaelbeaumont · 2018-11-12T10:26:19Z

It seems to me that it would be possible for Stream(1) to be closed before the push promise is sent out? The order in which frames are sent out for any given stream should not be assumed. The prioritization logic is free to change.

True, not a good solution.
I reverted it and am now tracking pending_push explicitly. That prevents the early sending of pushed response headers.
Added a few other tests for more complicated scenarios/bugs run into along the way.

There will need to be backpressure support for pushed requests similar to Client::send_request I think, right?

michaelbeaumont · 2019-01-05T13:50:25Z

@carllerche do you have time to take another look?

src/frame/headers.rs

src/server.rs

src/proto/streams/streams.rs

ivanceras · 2019-08-26T09:57:27Z

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?

le-invoker · 2019-09-10T10:49:29Z

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.

michaelbeaumont · 2019-09-10T12:41:26Z

Tests still need to be rewritten, I'll work on it

LimitlessCloud · 2019-09-12T03:15:28Z

Any news on this? I think thousands of people are expecting this crate to finally work since months but still nothing :/ Please anyone fork it and make it work, because HTTP2 is only useful because of SERVERPUSH... Save the rust community please

It's kinda mean to say that this is useless without this option...
It looks like the developer just abandoned the library that's it, I might work on a fork guys to help you, might be done in a few weeks.

seanmonstar · 2019-09-12T17:03:02Z

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!

src/server.rs

seanmonstar · 2019-09-12T17:42:50Z

src/server.rs

+        use http::request::Parts;
+
+        if let Err(_) = frame::PushPromise::validate_request(&request) {
+            return Err(UserError::MalformedHeaders);


Perhaps we should log the PushPromiseError, like at debug level.

seanmonstar

Looks good to me, thanks!

seanmonstar · 2019-09-13T21:13:59Z

Looks like there's some merge conflicts?

michaelbeaumont · 2019-09-16T12:27:07Z

@seanmonstar The branch is up to date as far as I can tell

seanmonstar · 2019-09-16T18:31:28Z

itshappening.gif

NN-Binary · 2019-09-16T23:43:15Z

Wow amazing job guys! Congrats, you are the first Rust crate to handle HTTP2 Server push!

le-invoker · 2019-09-18T02:28:08Z

Thank you all for the hardwork.
Now, if someone can pinpoint me the exact function to use for me to tell the webserver to create a server_push response for each corresponding future request.

le-invoker · 2019-09-27T02:56:02Z

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?

Add server support for push

e116ed6

Closes #291, closes #185

Properly queue push promise frames

287c36b

carllerche reviewed Oct 24, 2018

View reviewed changes

michaelbeaumont added 3 commits November 11, 2018 22:26

Revert "Properly queue push promise frames"

f2e0241

This reverts commit 287c36b.

Don't send for stream if a PushPromise is pending

151c0bf

Add additional tests

8a8c6ac

Add SendPushedResponse wrapper type

09a9ab6

hawkw reviewed Jan 11, 2019

View reviewed changes

src/frame/headers.rs Outdated Show resolved Hide resolved

src/server.rs Show resolved Hide resolved

src/proto/streams/streams.rs Outdated Show resolved Hide resolved

Code cleanup

f49a24f

michaelbeaumont added 2 commits September 10, 2019 23:29

Merge remote-tracking branch 'upstream/master' into server_push

810e56e

Update push promise based tests

c118a52

This comment has been minimized.

Sign in to view

michaelbeaumont requested a review from carllerche September 12, 2019 12:26

seanmonstar mentioned this pull request Sep 12, 2019

Where is the server push? #406

Closed

seanmonstar reviewed Sep 12, 2019

View reviewed changes

michaelbeaumont added 2 commits September 13, 2019 11:42

Fix and add comments, add debug messages

51c94dc

Merge remote-tracking branch 'upstream/master' into server_push

a1ca693

seanmonstar approved these changes Sep 13, 2019

View reviewed changes

seanmonstar merged commit fac165e into hyperium:master Sep 16, 2019

teohhanhui mentioned this pull request May 21, 2020

http/2 push actix/actix-web#150

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add server support for push #327

Add server support for push #327

michaelbeaumont commented Oct 21, 2018

michaelbeaumont commented Oct 21, 2018 •

edited

Loading

carllerche commented Oct 23, 2018

carllerche left a comment

carllerche Oct 24, 2018

michaelbeaumont Oct 24, 2018

carllerche Oct 24, 2018

carllerche Oct 24, 2018

carllerche commented Oct 24, 2018

michaelbeaumont commented Oct 24, 2018

carllerche commented Oct 29, 2018

michaelbeaumont commented Nov 12, 2018

michaelbeaumont commented Jan 5, 2019

ivanceras commented Aug 26, 2019

le-invoker commented Sep 10, 2019

michaelbeaumont commented Sep 10, 2019

This comment has been minimized.

LimitlessCloud commented Sep 12, 2019 •

edited

Loading

seanmonstar commented Sep 12, 2019

seanmonstar Sep 12, 2019

seanmonstar left a comment

seanmonstar commented Sep 13, 2019

michaelbeaumont commented Sep 16, 2019

seanmonstar commented Sep 16, 2019

NN-Binary commented Sep 16, 2019

le-invoker commented Sep 18, 2019

le-invoker commented Sep 27, 2019

Add server support for push #327

Add server support for push #327

Conversation

michaelbeaumont commented Oct 21, 2018

michaelbeaumont commented Oct 21, 2018 • edited Loading

carllerche commented Oct 23, 2018

carllerche left a comment

Choose a reason for hiding this comment

carllerche Oct 24, 2018

Choose a reason for hiding this comment

michaelbeaumont Oct 24, 2018

Choose a reason for hiding this comment

carllerche Oct 24, 2018

Choose a reason for hiding this comment

carllerche Oct 24, 2018

Choose a reason for hiding this comment

carllerche commented Oct 24, 2018

michaelbeaumont commented Oct 24, 2018

carllerche commented Oct 29, 2018

michaelbeaumont commented Nov 12, 2018

michaelbeaumont commented Jan 5, 2019

ivanceras commented Aug 26, 2019

le-invoker commented Sep 10, 2019

michaelbeaumont commented Sep 10, 2019

This comment has been minimized.

LimitlessCloud commented Sep 12, 2019 • edited Loading

seanmonstar commented Sep 12, 2019

seanmonstar Sep 12, 2019

Choose a reason for hiding this comment

seanmonstar left a comment

Choose a reason for hiding this comment

seanmonstar commented Sep 13, 2019

michaelbeaumont commented Sep 16, 2019

seanmonstar commented Sep 16, 2019

NN-Binary commented Sep 16, 2019

le-invoker commented Sep 18, 2019

le-invoker commented Sep 27, 2019

michaelbeaumont commented Oct 21, 2018 •

edited

Loading

LimitlessCloud commented Sep 12, 2019 •

edited

Loading