Skip to content

Conversation

addaleax
Copy link
Collaborator

First half of addressing MONGOSH-586.
Before this commit, what could happen in blocked mode is that:

  • There is data that was entered on the input stream but not yet
    released
  • Ctrl+C was received
  • The handler for released data would do something that leads
    to another _flush() call
  • This inner _flush() call would start emitting data while
    we were already doing so, which is not something that the
    readline implementation expects from its input stream
    (which is arguably a bug in Node.js)

Concretely, in the situation described in the ticket:

  1. Autocompleting during .editor puts the stream into blocking
    mode (the other half of the ticket)
  2. Pressing Ctrl+C to abort that means that the Ctrl+C character
    is forwarded immediately
  3. The readline implementation reads it and emits its internal
    'SIGINT' event
  4. The REPL implementation uses .displayPrompt() when that
    event is received
  5. Our .displayPrompt() handler explicitly calls nextLine()
  6. nextLine() leads to another _flush() call, which would then
    start emitting queued up data while the .push() call for the
    Ctrl+C is still on the stack
  7. The readline generator function for keypress events crashes
    because generators can’t have their .next() function called
    recursively

In order to fix this, we do not process ._flush() calls when
we are already inside a .push() call. This should be a reasonable
fix, because the situations in which we call .push() are:

  • Inside of _flush() itself, where it’s okay to skip _flush()
    calls because the input character queue is being processed already
  • Inside _forwardAndBlockOnNewline() for urgent events
    (Ctrl+C/Ctrl+D), where it’s also okay to skip _flush() calls
    because we call that immediately afterwards
  • Inside _forwardWithoutBlocking(), where the assumption would be
    that there is no character queue to be flushed in the first place

Unfortunately, this adds a bit of extra complexity to the already
somewhat complicated LineByLineInput implementation.

First half of addressing MONGOSH-586.
Before this commit, what could happen in blocked mode is that:

- There is data that was entered on the input stream but not yet
  released
- Ctrl+C was received
- The handler for released data would do something that leads
  to another `_flush()` call
- This inner `_flush()` call would start emitting data while
  we were already doing so, which is not something that the
  readline implementation expects from its input stream
  (which is arguably a bug in Node.js)

Concretely, in the situation described in the ticket:

1. Autocompleting during `.editor` puts the stream into blocking
   mode (the other half of the ticket)
2. Pressing Ctrl+C to abort that means that the Ctrl+C character
   is forwarded immediately
3. The readline implementation reads it and emits its internal
   'SIGINT' event
4. The REPL implementation uses `.displayPrompt()` when that
   event is received
5. Our `.displayPrompt()` handler explicitly calls `nextLine()`
6. `nextLine()` leads to another `_flush()` call, which would then
   start emitting queued up data while the `.push()` call for the
   `Ctrl+C` is still on the stack
7. The readline generator function for keypress events crashes
   because generators can’t have their `.next()` function called
   recursively

In order to fix this, we do not process `._flush()` calls when
we are already inside a `.push()` call. This should be a reasonable
fix, because the situations in which we call `.push()` are:

- Inside of `_flush()` itself, where it’s okay to skip `_flush()`
  calls because the input character queue is being processed already
- Inside `_forwardAndBlockOnNewline()` for urgent events
  (Ctrl+C/Ctrl+D), where it’s also okay to skip `_flush()` calls
  because we call that immediately afterwards
- Inside `_forwardWithoutBlocking()`, where the assumption would be
  that there is no character queue to be flushed in the first place

Unfortunately, this adds a bit of extra complexity to the already
somewhat complicated `LineByLineInput` implementation.
@addaleax addaleax changed the title fix(cli-repl): do not recursive push() in LineByLineInput MONGOSH-586 fix(cli-repl): do not recursively push() in LineByLineInput MONGOSH-586 Feb 15, 2021
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for a detailed description of what was happening there

@addaleax addaleax merged commit 2c7d950 into master Feb 15, 2021
@addaleax addaleax deleted the 586-dev-1 branch February 15, 2021 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants