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.

Sign up for GitHub

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

Jump to bottom

cli: write report in runLighthouse before quitting Chrome #7339

Merged
merged 3 commits into from
Mar 1, 2019
Merged

cli: write report in runLighthouse before quitting Chrome #7339

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd65cec
cli: convert runLighthouse to async/await
brendankenny Feb 27, 2019
868f306
add timeout
brendankenny Feb 27, 2019
e00b2cb
exit on failure to kill Chrome
brendankenny Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 57 additions & 57 deletions lighthouse-cli/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,53 +62,57 @@ function getDebuggableChrome(flags) {
});
}

/** @return {never} */
function showConnectionError() {
console.error('Unable to connect to Chrome');
process.exit(_RUNTIME_ERROR_CODE);
return process.exit(_RUNTIME_ERROR_CODE);
}

/** @return {never} */
function showProtocolTimeoutError() {
console.error('Debugger protocol timed out while connecting to Chrome.');
process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
return process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
}

/** @param {LH.LighthouseError} err */
/** @param {LH.LighthouseError} err @return {never} */
function showPageHungError(err) {
console.error('Page hung:', err.friendlyMessage);
process.exit(_PAGE_HUNG_EXIT_CODE);
return process.exit(_PAGE_HUNG_EXIT_CODE);
}

/** @param {LH.LighthouseError} err */
/** @param {LH.LighthouseError} err @return {never} */
function showInsecureDocumentRequestError(err) {
console.error('Insecure document request:', err.friendlyMessage);
process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE);
return process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE);
}

/**
* @param {LH.LighthouseError} err
* @return {never}
*/
function showRuntimeError(err) {
console.error('Runtime error encountered:', err.friendlyMessage || err.message);
if (err.stack) {
console.error(err.stack);
}
process.exit(_RUNTIME_ERROR_CODE);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the benefit of these return process.exit changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the benefit of these return process.exit changes?

purely tsc, you have to return it to take advantage of the @return {never} chain

return process.exit(_RUNTIME_ERROR_CODE);
}

/**
* @param {LH.LighthouseError} err
* @return {never}
*/
function handleError(err) {
if (err.code === 'ECONNREFUSED') {
showConnectionError();
return showConnectionError();
} else if (err.code === 'CRI_TIMEOUT') {
showProtocolTimeoutError();
return showProtocolTimeoutError();
} else if (err.code === 'PAGE_HUNG') {
showPageHungError(err);
return showPageHungError(err);
} else if (err.code === 'INSECURE_DOCUMENT_REQUEST') {
showInsecureDocumentRequestError(err);
return showInsecureDocumentRequestError(err);
} else {
showRuntimeError(err);
return showRuntimeError(err);
}
}

Expand Down Expand Up @@ -159,66 +163,62 @@ async function saveResults(runnerResult, flags) {
}
}

/**
* Attempt to kill the launched Chrome, if defined.
* @param {ChromeLauncher.LaunchedChrome=} launchedChrome
* @return {Promise<void>}
*/
async function potentiallyKillChrome(launchedChrome) {
if (!launchedChrome) return;

try {
await launchedChrome.kill();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we time this out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we time this out?

probably, but the Chrome child process still keeps the main process going, so it just prints a Couldn't quit Chrome process. Timed out. error message but still sits there spinning. Is that better?

We could also process.exit(1) in that case, but assuming we want to always exit in that case seems like it might be excessive?

} catch (err) {
process.stderr.write(`Couldn't quit Chrome process. ${err.toString()}\n`);
}
}

/**
* @param {string} url
* @param {LH.CliFlags} flags
* @param {LH.Config.Json|undefined} config
* @return {Promise<LH.RunnerResult|void>}
* @return {Promise<LH.RunnerResult|undefined>}
*/
function runLighthouse(url, flags, config) {
/** @type {ChromeLauncher.LaunchedChrome|undefined} */
let launchedChrome;
const shouldGather = flags.gatherMode || flags.gatherMode === flags.auditMode;
let chromeP = Promise.resolve();

process.on('unhandledRejection', async (reason) => {
async function runLighthouse(url, flags, config) {
/** @param {any} reason */
async function handleTheUnhandled(reason) {
process.stderr.write(`Unhandled Rejection. Reason: ${reason}\n`);
try {
await potentiallyKillChrome();
} catch (err) {
process.stderr.write(`Couldn't quit Chrome process. ${err.toString()}\n`);
}
await potentiallyKillChrome(launchedChrome);
setTimeout(_ => {
process.exit(1);
}, 100);
});

if (shouldGather) {
chromeP = chromeP.then(_ =>
getDebuggableChrome(flags).then(launchedChromeInstance => {
launchedChrome = launchedChromeInstance;
flags.port = launchedChrome.port;
})
);
}
process.on('unhandledRejection', handleTheUnhandled);

const resultsP = chromeP.then(_ => {
return lighthouse(url, flags, config).then(runnerResult => {
return potentiallyKillChrome().then(_ => runnerResult);
}).then(async runnerResult => {
// If in gatherMode only, there will be no runnerResult.
if (runnerResult) {
await saveResults(runnerResult, flags);
}
/** @type {ChromeLauncher.LaunchedChrome|undefined} */
let launchedChrome;

return runnerResult;
});
});
try {
const shouldGather = flags.gatherMode || flags.gatherMode === flags.auditMode;
if (shouldGather) {
launchedChrome = await getDebuggableChrome(flags);
flags.port = launchedChrome.port;
}

return resultsP.catch(err => {
return Promise.resolve()
.then(_ => potentiallyKillChrome())
.then(_ => handleError(err));
});
const runnerResult = await lighthouse(url, flags, config);

/**
* @return {Promise<{}>}
*/
function potentiallyKillChrome() {
if (launchedChrome !== undefined) {
return launchedChrome.kill();
// If in gatherMode only, there will be no runnerResult.
if (runnerResult) {
await saveResults(runnerResult, flags);
}
return Promise.resolve({});

await potentiallyKillChrome(launchedChrome);
process.removeListener('unhandledRejection', handleTheUnhandled);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be removed in catch too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be removed in catch too?

handleError always ends with a process.exit(), so it doesn't matter in the catch case. If we moved handleError to bin.js, though, we could put both potentiallyKillChrome and removing the listener to a finally block, which would be kind of nice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right 👍


return runnerResult;
} catch (err) {
await potentiallyKillChrome(launchedChrome);
handleError(err);
}
}

Expand Down