-
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
clients: exit CLI on unhandledRejection #6394
Conversation
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.
WFM
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.
runLighthouse
gets more convoluted, but it works :)
lighthouse-cli/run.js
Outdated
@@ -148,6 +148,14 @@ function runLighthouse(url, flags, config) { | |||
const shouldGather = flags.gatherMode || flags.gatherMode === flags.auditMode; | |||
let chromeP = Promise.resolve(); | |||
|
|||
process.on('unhandledRejection', async (reason) => { | |||
process.stderr.write(`Unhandled Rejection. Reason: ${reason}`); | |||
await potentiallyKillChrome(); |
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.
this turns into a runaway error in flaky appveyor which is already sometimes throwing unhandled rejections, then potentiallyKillChrome
in here ends up throwing another, which gets caught by this event handler which throws another which...
try/catching in here should be enough to stop it (maybe with an additional stderr
log that it happened)
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've really fallen into the trap 100% ignoring appveyor at this point, should we prioritize trying to get windows travis going? get an auto-restart bot? something else?
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.
should we prioritize trying to get windows travis going
I think we should. It feels like appveyor is going to continue to be like this, and azure isn't a near-term possibility because of some UI issues (were there other issues?).
Who knows, maybe it'll just work :) @paulirish might have had some ideas on this
@brendankenny try/catch up for the unhandled. |
to fix the behavior brendan pointed out here:
#6387 (review)