-
Notifications
You must be signed in to change notification settings - Fork 9.5k
core(session): ignore protocol timeouts from puppeteer #15512
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
const resultPromise = this._cdpSession.send(method, ...params).catch(err => { | ||
// We set up our own protocol timeout system, so we should ignore protocol timeouts emitted by puppeteer. | ||
// https://github.com/GoogleChrome/lighthouse/issues/15510 | ||
if (/'protocolTimeout'/.test(err)) return; |
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.
Won't this resolve with undefined
, though, if timeoutPromise
doesn't win the race?
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.
Good point. I guess if puppeteer gives us a protocol timeout error that means they are done processing the command. Even if we wait around longer, we won't get a result.
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.
I suppose an better solution would be to just ignore this type of error for our Page.navigate
call specifically? IDK but this clearly needs more work.
This won't work because puppeteer will cut off the command response even if Lighthouse is still waiting. I think what we need is for puppeteer to allow setting a timeout on individual commands. |
Fixes #15510