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

PUSH_PROMISE ordering guarantee #557

Closed
martinthomson opened this issue May 25, 2017 · 11 comments
Closed

PUSH_PROMISE ordering guarantee #557

martinthomson opened this issue May 25, 2017 · 11 comments
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.

Comments

@martinthomson
Copy link
Member

HTTP/2 has a pretty strong recommendation to have PUSH_PROMISE precede any text that mentions it. Failing to do so could mean that a client requests a resource that was pushed, which is wasteful.

Splitting the headers and data into two streams means that there is no way to order the PUSH_PROMISE relative to the part of the body that mentions the resource.

I'm sure that @MikeBishop mentioned this once before, but I couldn't find any issues for it. Returning to a model where data and headers share a stream would fix this.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -http labels May 25, 2017
@MikeBishop
Copy link
Contributor

Good point. Don't recall an issue on this specifically -- maybe it came up on the issue about whether to recombine them? Moot point until #176 is sorted out, though.

@krasic
Copy link

krasic commented May 25, 2017

GCUIC didn't have headers and data in the same stream, it just had all headers on stream 3. It enforced that a application reader receive header bytes before body bytes, and honored the recommendation wrt PUSH_PROMISE ordering. I don't think that should change under two streams per HTTP request.

@martinthomson
Copy link
Member Author

martinthomson commented May 26, 2017

@krasic, GQUIC has exactly the same issue. The body of a response can mention a resource and appear before a PUSH_PROMISE.

Unless, by this:

It enforced that a application reader receive header bytes before body bytes

...you mean that the stack was required to withhold stream data until stream 3 explicitly released it. Yes, we could do the same with split headers and body, but we can't do that generically, you have to have an explicit marker in the headers stream that says "you may read the body now". That's equivalent to just having data on the same stream as headers, with a bunch of extra complexity stacked on.

@MikeBishop, #175 :)

@MikeBishop
Copy link
Contributor

For a simple push application where the server generates HEADERS, PUSH_PROMISE, DATA*, that kind of works -- if the client knows to keep looking for PUSH_PROMISE frames as well. That effectively requires PUSH_PROMISE to be part of the header block.

For a more complex push application where the server generates HEADERS (PUSH_PROMISE? DATA*)*, there's no way for a client to know when it should return to pick up another PUSH_PROMISE. In the best case, it's just processing data as it arrives, and always treats bytes on the headers stream as higher-priority than bytes on the data stream. However, it's then subject to reordering -- or even partial arrival of a PUSH_PROMISE frame, given that QUIC doesn't know app-layer frame boundaries.

My unidirectional2 branch is just as vulnerable here, because it still allocates separate streams for each and keeps PUSH_PROMISE on the parent request's headers stream.

@krasic
Copy link

krasic commented May 26, 2017

@martinthomson yes, I did mean that the stack withholds stream data until the headers have been consumed.

@MikeBishop Thanks again for a great explanation. I didn't think about promises interleaved with data after the initial headers. I agree with your assessment. FWIW, I don't think either GQUIC nor Google's server-side HTTP/2 actually support generating promises post initial headers, so in that sense they aren't fully HTTP/2 spec compliant. I can envision use cases for streaming pushes, but not without a bunch of other stuff fixed (like cached digests etc.).

@MikeBishop
Copy link
Contributor

The primary case for us (as a server app platform) was a server generating and streaming back partial content as it continues to process. The HTTP/2 requirement is that the PUSH_PROMISE SHOULD come before the DATA or HEADERS frame which contains the reference, which isn't a hard requirement that PUSH_PROMISE precede all the data.

@krasic
Copy link

krasic commented May 26, 2017

I think by far the most common use of push in the wild is where the pull stream contains rel=preload headers (or similar) in its response headers, and the push promises precede said response headers. At least I know it is status quo for us.

I'd be curious to know about use cases with different patterns. I don't have a clear mapping of how your "server generating and streaming partial content as it continues to process" actually looks wrt server push.

@kazuho
Copy link
Member

kazuho commented Jun 8, 2017

@martinthomson

Returning to a model where data and headers share a stream would fix this.

To me it seems that sending PUSH_PROMISE as part of the parent stream is problematic regardless of whether or not headers and body are sent as separate streams.

Consider the case where a server is sending multiple responses that all refer to a single resource. This could happen for example when a client requests two HTML files to be shown in different iframes, and when the two HTML files refer to the same CSS file.

In such case, you would need to refer to the pushed stream from both of the responses. However, current design allows you to create a dependency to a pushed response from only one response. Therefore, we have a race condition if the client receives the responses of two or more streams (that both refer to a pushed resource) in a different order than the server sends.

To this end, I think that we should send PUSH_PROMISE using a different stream (possibly by using the stream that will convey the response headers), and within the parent stream only refer to that stream, instead of trying to convey the request headers of the pushed stream in the parent stream.

I understand that I am proposing to create dependency between streams. But I might argue that doing so (rather than trying to send PUSH_PROMISE in the parent stream) would benefit us in the long run, under the assumption that such dependency is expected to show up in application protocols other than HTTP as well.

@MikeBishop
Copy link
Contributor

MikeBishop commented Jun 8, 2017 via email

@kazuho
Copy link
Member

kazuho commented Jun 8, 2017

Note, though, that this still doesn’t solve ordering of the promise and the reference. It actually makes it worse, since while you know a push is coming, you don’t know what, and you still need to order that reference with respect to the body fragments.

Thank you for the response. That is a good point.

I would argue that if we decide to covey the request headers of a pushed response outside the parent stream, we should 1) unify headers and body into single stream, and 2) create a HTTP frame (that is sent as part of the unified stream) that defines a dependency (i.e.tell the client to wait for the headers of a different stream). If we go this route, I think that it would not be "worse" at least.

PS. Or if decide to split headers and body into different streams, then we could define a dependency between the two using the same method as we will do for the pushed streams.

@MikeBishop
Copy link
Contributor

Closed by #692.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

No branches or pull requests

4 participants