Skip to content

Commit df41dba

Browse files
committed
fix(cli-repl): only ever run MongoshRepl.onExit once
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.
1 parent 93761ce commit df41dba

File tree

1 file changed

+10
-3
lines changed

1 file changed

+10
-3
lines changed

packages/cli-repl/src/mongosh-repl.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ type Mutable<T> = {
127127
class MongoshNodeRepl implements EvaluationListener {
128128
_runtimeState: MongoshRuntimeState | null;
129129
closeTrace?: string;
130+
exitPromise?: Promise<never>;
130131
input: Readable;
131132
lineByLineInput: LineByLineInput;
132133
output: Writable;
@@ -545,6 +546,7 @@ class MongoshNodeRepl implements EvaluationListener {
545546
});
546547

547548
repl.on('exit', () => {
549+
if (this._runtimeState) this._runtimeState.repl = null;
548550
this.onExit().catch(() => {
549551
/* ... */
550552
});
@@ -1051,7 +1053,10 @@ class MongoshNodeRepl implements EvaluationListener {
10511053
if (rs) {
10521054
this._runtimeState = null;
10531055
this.closeTrace = new Error().stack;
1054-
rs.repl?.close();
1056+
if (rs.repl) {
1057+
rs.repl.close();
1058+
await once(rs.repl, 'exit');
1059+
}
10551060
await rs.instanceState.close(true);
10561061
await new Promise((resolve) => this.output.write('', resolve));
10571062
}
@@ -1063,8 +1068,10 @@ class MongoshNodeRepl implements EvaluationListener {
10631068
* @param exitCode The user-specified exit code, if any.
10641069
*/
10651070
async onExit(exitCode?: number): Promise<never> {
1066-
await this.close();
1067-
return this.ioProvider.exit(exitCode);
1071+
return (this.exitPromise ??= (async () => {
1072+
await this.close();
1073+
return this.ioProvider.exit(exitCode);
1074+
})());
10681075
}
10691076

10701077
/**

0 commit comments

Comments
 (0)