From 845e8481fbc802df923704b9ae92658dbed7790f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 11 Dec 2024 16:54:49 -0500 Subject: [PATCH 1/2] 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. --- packages/cli-repl/src/mongosh-repl.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index a977572c22..d1117e913b 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -127,6 +127,7 @@ type Mutable = { class MongoshNodeRepl implements EvaluationListener { _runtimeState: MongoshRuntimeState | null; closeTrace?: string; + exitPromise?: Promise; input: Readable; lineByLineInput: LineByLineInput; output: Writable; @@ -545,6 +546,7 @@ class MongoshNodeRepl implements EvaluationListener { }); repl.on('exit', () => { + if (this._runtimeState) this._runtimeState.repl = null; this.onExit().catch(() => { /* ... */ }); @@ -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)); } @@ -1063,8 +1068,10 @@ class MongoshNodeRepl implements EvaluationListener { * @param exitCode The user-specified exit code, if any. */ async onExit(exitCode?: number): Promise { - await this.close(); - return this.ioProvider.exit(exitCode); + return (this.exitPromise ??= (async () => { + await this.close(); + return this.ioProvider.exit(exitCode); + })()); } /** From 3bcdc5295025302154e06fa5f746f150aa5ca17e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 13 Dec 2024 08:46:12 -0500 Subject: [PATCH 2/2] fixup: cr comments --- packages/cli-repl/src/mongosh-repl.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index d1117e913b..80ee338189 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -546,6 +546,7 @@ class MongoshNodeRepl implements EvaluationListener { }); repl.on('exit', () => { + // repl is already closed, no need to close it again via this.close() if (this._runtimeState) this._runtimeState.repl = null; this.onExit().catch(() => { /* ... */ @@ -1036,7 +1037,7 @@ class MongoshNodeRepl implements EvaluationListener { throw new MongoshInternalError( `mongosh not initialized yet\nCurrent trace: ${ new Error().stack - }\nClose trace: ${this.closeTrace}\n` + }\nClose trace: ${this.closeTrace}` ); } return this._runtimeState; @@ -1054,6 +1055,7 @@ class MongoshNodeRepl implements EvaluationListener { this._runtimeState = null; this.closeTrace = new Error().stack; if (rs.repl) { + // Can be null if the repl already emitted 'exit' rs.repl.close(); await once(rs.repl, 'exit'); }