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 history to detect conflicting header field values #1864

Closed
dtikhonov opened this issue Oct 17, 2018 · 9 comments · Fixed by #2072
Closed

Push promise history to detect conflicting header field values #1864

dtikhonov opened this issue Oct 17, 2018 · 9 comments · Fixed by #2072
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.

Comments

@dtikhonov
Copy link
Member

draft-ietf-quic-http-15 has:

   []                           A client MUST treat receipt of a
   PUSH_PROMISE with conflicting header field values for the same Push
   ID as a connection error of type HTTP_MALFORMED_FRAME.

   Allowing duplicate references to the same Push ID is primarily to
   reduce duplication caused by concurrent requests.  A server SHOULD
   avoid reusing a Push ID over a long period.  Clients are likely to
   consume server push responses and not retain them for reuse over
   time.  Clients that see a PUSH_PROMISE that uses a Push ID that they
   have since consumed and discarded are forced to ignore the
   PUSH_PROMISE.

The two paragraphs contradict each other. The former mandates that a client maintain a history of all push promises in order to detect possible header field conflicts. The latter suggests that a client does not have to do that. Which one is it?

The requirement to maintain a history of all push requests is unreasonable. I suggest that the MUST in the first paragraph above be replaced with a MAY.

@LPardue
Copy link
Member

LPardue commented Oct 17, 2018

I agree.

I've never considered this before but now I'm thinking that a client would need to store the non-compressed version of PUSH_PROMISE, in case an instruction elsewhere caused the dynamic table to be modified.

@MikeBishop
Copy link
Contributor

As in other places, the intent isn't to require historical tracking and verification forever, but to require the error if the condition is detected while acknowledging that it might be. I think, more precisely, what we want is that the client:

  • Maintains a list of active promises for which it is still willing to accept an additional reference to the promise, including the uncompressed headers for the promise (likely until the promise is fulfilled, possibly plus a timeout)
  • Verifies that the headers match exactly when receiving an additional reference to one of these promises; MUST error if they don't.
  • If a promise is received for an ID which has already been flushed from the list, the promise is ignored

This entails more text than just changing MUST to MAY, but is this a more precise description of what we're after?

@LPardue
Copy link
Member

LPardue commented Oct 17, 2018

With the way @MikeBishop puts it, perhaps it is useful to describe the problem that header field changes might cause, then provide the points on how a client might want to do things to detect and avoid them.

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

MikeBishop commented Nov 29, 2018

I kind of wish the pushed request headers only got sent once so that this conflict could never occur. What if we instead had a DUPLICATE_PUSH frame that just includes the Push ID and means "same thing here"? If it arrives before the corresponding PUSH_PROMISE, you have to wait to find out the request headers, but they're hopefully coming soon.

As a corollary, receiving a second PUSH_PROMISE with the same Push ID would then be an error regardless of contents.

@dtikhonov
Copy link
Member Author

I like it.

@martinthomson
Copy link
Member

Seems reasonable.

@RyanTheOptimist
Copy link
Contributor

RyanTheOptimist commented Dec 1, 2018 via email

@MikeBishop
Copy link
Contributor

No, it's a trade-off and that's the down-side. The fact that it's got a DUPLICATE_PUSH could cause it to briefly delay requests for URLs it discovers in hopes that the PUSH_PROMISE arrives soon, or perhaps it issues requests and then cancels them when the PUSH_PROMISE arrives. I'd love to have some first-order hint, like the resource URI, but that's painfully cross-layer.

Of course, the same situation exists today, albeit with lower odds. If the PUSH_PROMISE headers are blocked in the QPACK encoder, but you choose to buffer the frame so you can keep reading the response, what do you do currently? If you block reading on the response, that's even worse than failing to send requests to the network....

@RyanTheOptimist
Copy link
Contributor

RyanTheOptimist commented Dec 1, 2018 via email

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

Successfully merging a pull request may close this issue.

5 participants