Skip to content

Commit

Permalink
fix(cli-repl): only ever run MongoshRepl.onExit once
Browse files Browse the repository at this point in the history
Previously, a task in our CI was flaky because it was running `exit\n`
as literal input to the shell, expecting it to reliably make the shell exit.

`exit` as a command leads to the `MongoshRepl.onExit` function called,
which then calls a) `.close()` and, indirectly, `repl.close()` on the
Node.js REPL instance as well b) the `CliRepl.exit` function (and through
that, indirectly, `process.exit()`).

However, closing the Node.js REPL instance makes it flush the history
file to disk as a typically fast but asynchronous operation, and,
after that, emit `exit` on the REPL; that in turn called
`MongoshRepl.onExit` again (i.e. calling `MongoshRepl.onExit` leads
to another call to the same function, just asynchronously with
a short delay).

The flaky test in CI failed because of a combination of issues,
including the fact that `process.exit()` did not necessarily actually
exit the process because of coverage tooling interfering with it
(which in turn happened because it was running with an altered
working directory pointing at a temporary directory where `nyc`
had not created a subdirectory for storing coverage output).

In typical operations, the REPL would flush history quickly, and
calling `.onExit()` again would allow the process to exit
normally (by ways of calling `CliRepl.exit` and `process.exit()`
a second time). However, in the flaky failure case, the history
flushing operation would take long enough that the original
`process.exit()` call (which failed due to nyc) would set the
internal `process._exiting` flag before throwing, which in turn
prevents `process.nextTick()` from scheduling callbacks, and which
then ultimately prevented the REPL from cleaning itself up,
leading to the process timing out instead of exiting.

This commit introduces safeguards against calling `.onExit()` twice,
always returning the same (never-resolving) `Promise` instance,
and explicitly keeps track of whether the REPL was closed or not.
  • Loading branch information
addaleax committed Dec 12, 2024
1 parent 93761ce commit df41dba
Showing 1 changed file with 10 additions and 3 deletions.
13 changes: 10 additions & 3 deletions packages/cli-repl/src/mongosh-repl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ type Mutable<T> = {
class MongoshNodeRepl implements EvaluationListener {
_runtimeState: MongoshRuntimeState | null;
closeTrace?: string;
exitPromise?: Promise<never>;
input: Readable;
lineByLineInput: LineByLineInput;
output: Writable;
Expand Down Expand Up @@ -545,6 +546,7 @@ class MongoshNodeRepl implements EvaluationListener {
});

repl.on('exit', () => {
if (this._runtimeState) this._runtimeState.repl = null;
this.onExit().catch(() => {
/* ... */
});
Expand Down Expand Up @@ -1051,7 +1053,10 @@ class MongoshNodeRepl implements EvaluationListener {
if (rs) {
this._runtimeState = null;
this.closeTrace = new Error().stack;
rs.repl?.close();
if (rs.repl) {
rs.repl.close();
await once(rs.repl, 'exit');
}
await rs.instanceState.close(true);
await new Promise((resolve) => this.output.write('', resolve));
}
Expand All @@ -1063,8 +1068,10 @@ class MongoshNodeRepl implements EvaluationListener {
* @param exitCode The user-specified exit code, if any.
*/
async onExit(exitCode?: number): Promise<never> {
await this.close();
return this.ioProvider.exit(exitCode);
return (this.exitPromise ??= (async () => {
await this.close();
return this.ioProvider.exit(exitCode);
})());
}

/**
Expand Down

0 comments on commit df41dba

Please sign in to comment.