-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(network-recorder): handle QUIC requests #5256
Conversation
@@ -107,9 +107,15 @@ class NetworkRecorder extends EventEmitter { | |||
return; | |||
} | |||
|
|||
// QUIC network requests don't always "finish" even when they're done loading data | |||
// Use receivedHeaders instead, see https://github.com/GoogleChrome/lighthouse/issues/5254 | |||
const isQUIC = record._responseHeaders && record._responseHeaders |
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.
would be nice pulled out into a function to separate determining if request is QUIC vs what to do with QUIC requests
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.
it could also be a function that's like isQuicAndFinished(record)
to absorb logic below, too :)
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.
sure
// QUIC network requests don't always "finish" even when they're done loading data | ||
// Use receivedHeaders instead, see https://github.com/GoogleChrome/lighthouse/issues/5254 | ||
const isQUIC = record._responseHeaders && record._responseHeaders | ||
.find(header => header.name.toLowerCase() === 'alt-svc' && /quic/.test(header.value)); |
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.
how certain is the 'alt-svc'
check? Any other way to tell?
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.
seems to be fairly certain, in the spec it's not technically required to announce it buuuuuut...
While HTTP/QUIC doesn't formally require a client to implement Alt-Svc, there's no discovery mechanism other than Alt-Svc provided, so you're not going to get very far without
it.
@@ -119,7 +125,7 @@ class NetworkRecorder extends EventEmitter { | |||
.sort((a, b) => a.time - b.time); | |||
|
|||
let numInflightRequests = 0; | |||
let quietPeriodStart = 0; | |||
let quietPeriodStart = -Infinity; |
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.
what's the advantage of a [-Infinity, 0] period? Time starting at 0 also seems like a reasonable state of things
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.
that's fine just made tests have [0, 0] quiet period so we either handle the empty period case, move test cases to past 0, or -Infinity, -Infinity seemed like most reasonable conceptually but I do not have strong feelings
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.
it may be fine, and I haven't spent enough time here recently to have a good gut instinct, but I think I would expect the earliest quiet period to start at 0.
I guess I don't understand what this is addressing. Seems independent of the QUIC change?
is this something we should also fix in the upstream |
DevTools frontend handles this case correctly, I didn't dive into the code there to see how since its ~2 years or so diff :) |
closes #5254