From c4342592e90252c5c985ff39a3fd722cf69237f0 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna.henningsen@mongodb.com>
Date: Mon, 15 Feb 2021 14:20:05 +0100
Subject: [PATCH] fix(cli-repl): do not recursive push() in LineByLineInput
 MONGOSH-586
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../cli-repl/src/line-by-line-input.spec.ts    | 18 ++++++++++++++++++
 packages/cli-repl/src/line-by-line-input.ts    | 17 ++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/packages/cli-repl/src/line-by-line-input.spec.ts b/packages/cli-repl/src/line-by-line-input.spec.ts
index 1614e9c4b7..f6e595d23c 100644
--- a/packages/cli-repl/src/line-by-line-input.spec.ts
+++ b/packages/cli-repl/src/line-by-line-input.spec.ts
@@ -63,4 +63,22 @@ describe('LineByLineInput', () => {
       expect(forwardedChunks).to.deep.equal(['a', 'b', '\n']);
     });
   });
+
+  context('when a data listener calls nextLine() itself after Ctrl+C', () => {
+    it('does not emit data while already emitting data', () => {
+      let dataCalls = 0;
+      let insideDataCalls = 0;
+      lineByLineInput.on('data', () => {
+        expect(insideDataCalls).to.equal(0);
+        insideDataCalls++;
+        if (dataCalls++ === 0) {
+          lineByLineInput.nextLine();
+        }
+        insideDataCalls--;
+      });
+      stdinMock.emit('data', Buffer.from('foo\n\u0003'));
+      expect(dataCalls).to.equal(5);
+      expect(forwardedChunks).to.deep.equal(['\u0003', 'f', 'o', 'o', '\n']);
+    });
+  });
 });
diff --git a/packages/cli-repl/src/line-by-line-input.ts b/packages/cli-repl/src/line-by-line-input.ts
index aed8644dd0..c9982133c9 100644
--- a/packages/cli-repl/src/line-by-line-input.ts
+++ b/packages/cli-repl/src/line-by-line-input.ts
@@ -27,6 +27,7 @@ export class LineByLineInput extends Readable {
   private _blockOnNewLineEnabled: boolean;
   private _charQueue: (string | null)[];
   private _decoder: StringDecoder;
+  private _insidePushCalls: number;
 
   constructor(readable: NodeJS.ReadStream) {
     super();
@@ -35,6 +36,7 @@ export class LineByLineInput extends Readable {
     this._blockOnNewLineEnabled = true;
     this._charQueue = [];
     this._decoder = new StringDecoder('utf-8');
+    this._insidePushCalls = 0;
 
     const isReadableEvent = (name: string) =>
       name === 'readable' || name === 'data' || name === 'end' || name === 'keypress';
@@ -144,7 +146,11 @@ export class LineByLineInput extends Readable {
       // have in the buffer if in the meanwhile something
       // downstream explicitly called pause(), as that may cause
       // unexpected behaviors.
-      !this._originalInput.isPaused()
+      !this._originalInput.isPaused() &&
+
+      // If we are already inside a push() call, then we do not need to flush
+      // the queue again.
+      this._insidePushCalls === 0
     ) {
       const char = this._charQueue.shift() as string | null;
 
@@ -167,4 +173,13 @@ export class LineByLineInput extends Readable {
   private _isCtrlC(char: string | null): boolean {
     return char === CTRL_C;
   }
+
+  push(chunk: Buffer | string | null): boolean {
+    this._insidePushCalls++;
+    try {
+      return super.push(chunk);
+    } finally {
+      this._insidePushCalls--;
+    }
+  }
 }