Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 8 users
Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 750107



Sign in to add a comment
Implement correct request matching for pushed responses
Project Member Reported by tombergan@chromium.org, Nov 10 2015 Back to list
When matching a request to pushed responses, Chrome needs to validate all relevant headers to ensure the pushed response is a valid match for the request. In particular, if the response contains a non-empty Vary header, the varied headers in the request should match those in the response's PUSH_PROMISE.

For further discussion, see here:
https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/kuPNwmKZWzg
 
Cc: ckrasic@chromium.org
Comment 2 by b...@chromium.org, Apr 15 2016
Status: Available
Cc: tombergan@chromium.org
Cc: zhongyi@chromium.org
Comment 5 by ckrasic@google.com, Oct 5 2016
QUIC implementation of server push respects the Vary header.
Comment 6 by y...@yoav.ws, Nov 29 2016
Cc: y...@yoav.ws
In addition to Vary, a PUSH_PROMISE can in theory be a Range request that gets a pushed 206 response. Chrome needs to support this as well, at minimum by matching the Range header of the request with the Range header from the PUSH_PROMISE.

ckrasic@ -- From looking at the code (linked below), it looks like QUIC supports pushed responses with Vary but not pushed 206 responses?
https://cs.chromium.org/chromium/src/net/quic/core/quic_client_promised_info.cc?rcl=d25bb92682c7fb325d1f2b5608832a195fb0dd27&l=80
tombergan@ - correct, QUIC's push support does not consider range requests.
Cc: bol...@chromium.org
 Issue 727655  has been merged into this issue.
Cc: b...@chromium.org
+bnc@chromium.org
Project Member Comment 13 by bugdroid1@chromium.org, Jul 26
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/602c40ce2fb52c32518c074ebe56c2795d60a4bd

commit 602c40ce2fb52c32518c074ebe56c2795d60a4bd
Author: Bence Béky <bnc@chromium.org>
Date: Wed Jul 26 05:22:56 2017

Small cleanup in HTTP/2 and QUIC server push code.

* Remove unused QuicChromiumClientSession::GetStreamIdForPush().
* Remove unused SpdySession::GetStreamIdForPush().
* Add early returns in SpdySession::GetPushStream().
* Minor cleanup in SpdySession::TryCreatePushStream().
* Better error messages; change tests accordingly.

No functional change except for more informative error messages
(both in LOG() and in GOAWAY frames).

BUG=554220

Change-Id: If7d67c33d291a9dd2bd5b3721cf65aa17a7134d9
Reviewed-on: https://chromium-review.googlesource.com/584009
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489557}
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/spdy/chromium/spdy_session.h

Blocking: 750107
rdsmith@ pointed out that if in the vast majority of cases (~99%) the pushed response headers are available at the time when the request is matched up with the pushed stream, then it might not be worth the code complexity to do async magic for request matching: instead, in the few cases when response headers are not available, the pushed stream can be ignored.  (It is quite possible that this is the case, since the server or reverse proxy might have the response cached, so the response headers might even be in the same TCP packet as the PUSH_PROMISE.)  I'm therefore adding a histogram to measure this.
Re: comment #15.  I don't buy it.  

AFAIK, the common case is that server triggers the generation of pushes based on headers (preload, or older x-associated-content).   For each pushed resource, it has to initiate the corresponding request to backend.  So this races with PUSH_PROMISE and HEADERS that refer to the push on the way to client.  Client would be expected to process headers ASAP, which should detect the preload/associated headers, causing the corresponding requests to be generated. 

For the case that response headers are available, it has to be that server initiating response to backend and subsequently forwarding response to client is faster than client reading header and preload generating a request to rendezvous with the pushed response.  The former involves network communication, the latter is all local to the client, albeit with some IPCs within Chrome.   

I would think the case where client wins the race *should* be the common one.
Project Member Comment 17 by bugdroid1@chromium.org, Aug 28
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae145e73456bf7520da710f62078508bd30faa15

commit ae145e73456bf7520da710f62078508bd30faa15
Author: Bence Béky <bnc@chromium.org>
Date: Mon Aug 28 20:02:13 2017

Measure if response headers are available when matching pushed stream.

Add Net.PushedStreamAlreadyHasResponseHeaders histogram to measure
if response headers are available when matching a request to a pushed
stream.

BUG=554220

Change-Id: I94902e59f4f7c1624c3a3ed421412a39bd16275a
Reviewed-on: https://chromium-review.googlesource.com/635811
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497856}
[modify] https://crrev.com/ae145e73456bf7520da710f62078508bd30faa15/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/ae145e73456bf7520da710f62078508bd30faa15/tools/metrics/histograms/histograms.xml

Re: comment #16.  I find your argument plausible, and would not be surprised if you were right.  A histogram just landed to measure this, so we should find out soon.
FYI data so far show that 78% of claimed pushed streams have already received response headers by the time of rendezvous.  This is not high enough that we could afford dropping the remaining 22% of pushed streams, so I'll implement the async matching.
Sign in to add a comment