Skip to content

Commit

Permalink
Don't force-exit after tests have completed
Browse files Browse the repository at this point in the history
Fixes #1718.

With worker threads, this seems to stop AVA from detecting that the thread has exited, causing a hang.

Also remove flush logic implemented in #1722. Let's hope that current Node.js versions are better at flushing IPC before exiting.
  • Loading branch information
novemberborn committed Nov 12, 2023
1 parent e07179b commit e78d65c
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 45 deletions.
5 changes: 0 additions & 5 deletions lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,6 @@ export default function loadFork(file, options, execArgv = process.execArgv) {
break;
}

case 'ping': {
send({type: 'pong'});
break;
}

default: {
emitStateChange(message.ava);
}
Expand Down
6 changes: 6 additions & 0 deletions lib/reporters/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,12 @@ export default class Reporter {
break;
}

case 'exit-failed': {
this.write(colors.error(`${figures.cross} Failed to exit when running ${this.relativeFile(event.testFile)}`));

break;
}

case 'hook-finished': {
if (event.logs.length > 0) {
this.lineWriter.writeLine(` ${this.prefixTitle(event.testFile, event.title)}`);
Expand Down
57 changes: 33 additions & 24 deletions lib/worker/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,35 +23,42 @@ import {isRunningInThread, isRunningInChildProcess} from './utils.cjs';
const currentlyUnhandled = setUpCurrentlyUnhandled();
let runner;

let failedToExit = false;

const expectExit = code => {
// Set the exit code, which is just handy.
process.exitCode = code;

// Note that this timer is unreferenced. It should not keep the event loop
// open. However if after a second the process is still running, assume
// something is keeping the event loop busy. Exit forcibly but try to convey
// the reason to the main process.
nowAndTimers.setTimeout(() => {
failedToExit = true;
channel.send({type: 'exit-failed', code});
process.exit(1);
}, 1000).unref();
};

// Override process.exit with an undetectable replacement
// to report when it is called from a test (which it should never be).
const {apply} = Reflect;
const realExit = process.exit;

async function exit(code, forceSync = false) {
const flushing = channel.flush();
if (!forceSync) {
await flushing;
const handleProcessExit = (target, thisArg, args) => {
if (!failedToExit) {
const error = new Error('Unexpected process.exit()');
Error.captureStackTrace(error, handleProcessExit);
channel.send({type: 'process-exit', stack: error.stack});
}

apply(realExit, process, [code]);
}

const handleProcessExit = (fn, receiver, args) => {
const error = new Error('Unexpected process.exit()');
Error.captureStackTrace(error, handleProcessExit);
channel.send({type: 'process-exit', stack: error.stack});

// Make sure to extract the code only from `args` rather than e.g. `Array.prototype`.
// This level of paranoia is usually unwarranted, but we're dealing with test code
// that has already colored outside the lines.
const code = args.length > 0 ? args[0] : undefined;

// Force a synchronous exit as guaranteed by the real process.exit().
exit(code, true);
target.call(thisArg, code);
};

process.exit = new Proxy(realExit, {
process.exit = new Proxy(process.exit, {
apply: handleProcessExit,
});

Expand Down Expand Up @@ -101,7 +108,7 @@ const run = async options => {

runner.on('error', error => {
channel.send({type: 'internal-error', err: serializeError(error)});
exit(1);
expectExit(1);
});

runner.on('finish', async () => {
Expand All @@ -112,30 +119,32 @@ const run = async options => {
}
} catch (error) {
channel.send({type: 'internal-error', err: serializeError(error)});
exit(1);
expectExit(1);
return;
}

try {
await Promise.all(sharedWorkerTeardowns.map(fn => fn()));
} catch (error) {
channel.send({type: 'uncaught-exception', err: serializeError(error)});
exit(1);
expectExit(1);
return;
}

nowAndTimers.setImmediate(() => {
let exitCode = 0;
for (const rejection of currentlyUnhandled()) {
channel.send({type: 'unhandled-rejection', err: serializeError(rejection.reason, {testFile: options.file})});
exitCode = 1;
}

exit(0);
expectExit(exitCode);
});
});

process.on('uncaughtException', error => {
channel.send({type: 'uncaught-exception', err: serializeError(error, {testFile: options.file})});
exit(1);
expectExit(1);
});

// Store value to prevent required modules from modifying it.
Expand Down Expand Up @@ -248,11 +257,11 @@ const run = async options => {
channel.unref();
} else {
channel.send({type: 'missing-ava-import'});
exit(1);
expectExit(1);
}
} catch (error) {
channel.send({type: 'uncaught-exception', err: serializeError(error, {testFile: options.file})});
exit(1);
expectExit(1);
}
};

Expand Down
16 changes: 0 additions & 16 deletions lib/worker/channel.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -110,22 +110,6 @@ exports.peerFailed = selectAvaMessage(handle.channel, 'peer-failed');
exports.send = handle.send.bind(handle);
exports.unref = handle.unref.bind(handle);

let pendingPings = Promise.resolve();
async function flush() {
handle.ref();
const promise = pendingPings.then(async () => {
handle.send({type: 'ping'});
await selectAvaMessage(handle.channel, 'pong');
if (promise === pendingPings) {
handle.unref();
}
});
pendingPings = promise;
await promise;
}

exports.flush = flush;

let channelCounter = 0;
let messageCounter = 0;

Expand Down

0 comments on commit e78d65c

Please sign in to comment.