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.

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

fix: ensure that exit code for fatal errors is not overwritten #17683

Merged
merged 3 commits into from Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 24 additions & 2 deletions bin/eslint.js
Expand Up @@ -97,16 +97,22 @@ function getErrorMessage(error) {
* same message once.
* @type {Set<string>}
*/

const displayedErrors = new Set();

/**
* Tracks whether an unexpected error was caught
* @type {boolean}
*/
let hadFatalError = false;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some comments around these additions?


/**
* Catch and report unexpected error.
* @param {any} error The thrown error object.
* @returns {void}
*/
function onFatalError(error) {
process.exitCode = 2;
hadFatalError = true;

const { version } = require("../package.json");
const message = `
Expand Down Expand Up @@ -143,9 +149,25 @@ ${getErrorMessage(error)}`;
}

// Otherwise, call the CLI.
process.exitCode = await require("../lib/cli").execute(
const exitCode = await require("../lib/cli").execute(
process.argv,
process.argv.includes("--stdin") ? await readStdin() : null,
true
);

/*
* If an uncaught exception or unhandled rejection was detected in the meantime,
* keep the fatal exit code 2 that is already assigned to `process.exitCode`.
* Without this condition, exit code 2 (unsuccessful execution) could be overwritten with
* 1 (successful execution, lint problems found) or even 0 (successful execution, no lint problems found).
* This ensures that unexpected errors that seemingly don't affect the success
* of the execution will still cause a non-zero exit code, as it's a common
* practice and the default behavior of Node.js to exit with non-zero
* in case of an uncaught exception or unhandled rejection.
*
* Otherwise, assign the exit code returned from CLI.
*/
if (!hadFatalError) {
process.exitCode = exitCode;
}
}()).catch(onFatalError);
18 changes: 18 additions & 0 deletions tests/bin/eslint.js
Expand Up @@ -387,6 +387,24 @@ describe("bin/eslint.js", () => {
return Promise.all([exitCodeAssertion, outputAssertion]);
});

it("does not exit with zero when there is an error in the next tick", () => {
const config = path.join(__dirname, "../fixtures/bin/eslint.config-promise-tick-throws.js");
const file = path.join(__dirname, "../fixtures/bin/empty.js");
const child = runESLint(["--config", config, file]);
const exitCodeAssertion = assertExitCode(child, 2);
const outputAssertion = getOutput(child).then(output => {

// ensure the expected error was printed
assert.include(output.stderr, "test_error_stack");

// ensure that linting the file did not cause an error
assert.notInclude(output.stderr, "empty.js");
assert.notInclude(output.stdout, "empty.js");
});

return Promise.all([exitCodeAssertion, outputAssertion]);
});

// https://github.com/eslint/eslint/issues/17560
describe("does not print duplicate errors in the event of a crash", () => {

Expand Down
Empty file added tests/fixtures/bin/empty.js
Empty file.
22 changes: 22 additions & 0 deletions tests/fixtures/bin/eslint.config-promise-tick-throws.js
@@ -0,0 +1,22 @@
function throwError() {
const error = new Error();
error.stack = "test_error_stack";
throw error;
}

process.nextTick(throwError);

function delay(ms) {
return new Promise(resolve => setTimeout(resolve, ms));
}

async function getConfig() {
await delay(100);
return [];
}

/*
* Exporting the config as an initially unsettled Promise should ensure that
* the error in next tick will be thrown before any linting is done
*/
module.exports = getConfig();