Skip to content

Commit 18a499e

Browse files
authored
fix(cli-repl): only ever run MongoshRepl.onExit once MONGOSH-1943 (#2300)
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 5405fe4 commit 18a499e

File tree

1 file changed

+13
-4
lines changed

1 file changed

+13
-4
lines changed

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

Lines changed: 13 additions & 4 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,8 @@ class MongoshNodeRepl implements EvaluationListener {
545546
});
546547

547548
repl.on('exit', () => {
549+
// repl is already closed, no need to close it again via this.close()
550+
if (this._runtimeState) this._runtimeState.repl = null;
548551
this.onExit().catch(() => {
549552
/* ... */
550553
});
@@ -1034,7 +1037,7 @@ class MongoshNodeRepl implements EvaluationListener {
10341037
throw new MongoshInternalError(
10351038
`mongosh not initialized yet\nCurrent trace: ${
10361039
new Error().stack
1037-
}\nClose trace: ${this.closeTrace}\n`
1040+
}\nClose trace: ${this.closeTrace}`
10381041
);
10391042
}
10401043
return this._runtimeState;
@@ -1051,7 +1054,11 @@ class MongoshNodeRepl implements EvaluationListener {
10511054
if (rs) {
10521055
this._runtimeState = null;
10531056
this.closeTrace = new Error().stack;
1054-
rs.repl?.close();
1057+
if (rs.repl) {
1058+
// Can be null if the repl already emitted 'exit'
1059+
rs.repl.close();
1060+
await once(rs.repl, 'exit');
1061+
}
10551062
await rs.instanceState.close(true);
10561063
await new Promise((resolve) => this.output.write('', resolve));
10571064
}
@@ -1063,8 +1070,10 @@ class MongoshNodeRepl implements EvaluationListener {
10631070
* @param exitCode The user-specified exit code, if any.
10641071
*/
10651072
async onExit(exitCode?: number): Promise<never> {
1066-
await this.close();
1067-
return this.ioProvider.exit(exitCode);
1073+
return (this.exitPromise ??= (async () => {
1074+
await this.close();
1075+
return this.ioProvider.exit(exitCode);
1076+
})());
10681077
}
10691078

10701079
/**

0 commit comments

Comments
 (0)