fix(cli-repl): only ever run MongoshRepl.onExit once MONGOSH-1943 #2300
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gagik
approved these changes
Dec 13, 2024
| throw new MongoshInternalError( | ||
| `mongosh not initialized yet\nCurrentTrace: ${ | ||
| new Error().stack | ||
| }\nClose trace: ${this.closeTrace}\n` |
There was a problem hiding this comment.
is there a reason we're ending with a newline?
There was a problem hiding this comment.
Yeah no, good point. I think I have a tendency to always terminate string that contain multiple newlines with one (just like most editors do for files), but Errors are a bit special in that their messages are usually displayed with an extra newline anyway. Removed in 3bcdc52!
| @@ -544,6 +546,7 @@ class MongoshNodeRepl implements EvaluationListener { | |||
| }); | |||
|
|
|||
| repl.on('exit', () => { | |||
| if (this._runtimeState) this._runtimeState.repl = null; | |||
There was a problem hiding this comment.
what are the implications of this being set to null? is this to force a quicker garbage collection / flushing? might be worth leaving a comment, though perhaps I could just be lacking context
There was a problem hiding this comment.
Oh yeah, comment is a good idea – also added in 3bcdc52. (The shell can also exit when stdin ends, which would result in us calling this listener here first, before we call .onExit() through some other means)
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.
addaleax
force-pushed
the
1943-mongoshrepl-onexit
branch
from
df41dba to
3bcdc52
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Depends on #2299 as the first commit here and on #2298 logically – without #2298, this just fails consistently now instead of flakily)
Previously, a task in our CI was flaky because it was running
exit\nas literal input to the shell, expecting it to reliably make the shell exit.exitas a command leads to theMongoshRepl.onExitfunction called, which then calls a).close()and, indirectly,repl.close()on the Node.js REPL instance as well b) theCliRepl.exitfunction (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
exiton the REPL; that in turn calledMongoshRepl.onExitagain (i.e. callingMongoshRepl.onExitleads 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 wherenychad 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 callingCliRepl.exitandprocess.exit()a second time). However, in the flaky failure case, the history flushing operation would take long enough that the originalprocess.exit()call (which failed due to nyc) would set the internalprocess._exitingflag before throwing, which in turn preventsprocess.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)Promiseinstance, and explicitly keeps track of whether the REPL was closed or not.