From 5657d331b84b3f40d560ca6e5e0f3a227276082d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 4 May 2021 14:09:54 +0200 Subject: [PATCH 1/4] fix(cli-repl): work around double-prompt Node.js REPL bug MONGOSH-727 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the bug has more implications than making our tests flaky, start working around it instead of skipping tests. In particular, this fixes the linked ticket, because the REPL implementation calling `repl.displayPrompt()` twice after an error would also call `lineByLineInput.nextLine()` twice in our code, which would start two async evaluations happening at the same time, which breaks the async REPL wrapper’s API contract. --- packages/cli-repl/src/mongosh-repl.spec.ts | 6 ++++ packages/cli-repl/src/mongosh-repl.ts | 39 ++++++++++++++++++++-- packages/cli-repl/test/e2e.spec.ts | 4 +-- packages/cli-repl/test/repl-helpers.ts | 16 +-------- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/packages/cli-repl/src/mongosh-repl.spec.ts b/packages/cli-repl/src/mongosh-repl.spec.ts index 581ccf0fcd..4bdc13a0c8 100644 --- a/packages/cli-repl/src/mongosh-repl.spec.ts +++ b/packages/cli-repl/src/mongosh-repl.spec.ts @@ -220,6 +220,12 @@ describe('MongoshNodeRepl', () => { await waitEval(bus); expect(output).to.include('ISODate("2021-05-04T15:49:33.000Z")'); }); + + it('handles a long series of errors', async function() { + input.write('-asdf();\n'.repeat(20)); + await waitEval(bus); + expect(mongoshRepl.runtimeState().repl.listenerCount('SIGINT')).to.equal(1); + }); }); context('with terminal: true', () => { diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index a104d2b115..915762ae0d 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -9,8 +9,8 @@ import askpassword from 'askpassword'; import { Console } from 'console'; import { once } from 'events'; import prettyRepl from 'pretty-repl'; -import { ReplOptions, REPLServer } from 'repl'; -import type { Readable, Writable } from 'stream'; +import { ReplOptions, REPLServer, start as replStart } from 'repl'; +import { Readable, Writable, PassThrough } from 'stream'; import type { ReadStream, WriteStream } from 'tty'; import { callbackify, promisify } from 'util'; import * as asyncRepl from './async-repl'; @@ -56,6 +56,40 @@ type Mutable = { -readonly[P in keyof T]: T[P] }; +// https://github.com/nodejs/node/pull/38314 +function fixupReplForNodeBug38314(repl: REPLServer): void { + { + // Check whether bug is present: + const input = new PassThrough(); + const output = new PassThrough(); + const evalFn = (code, ctx, filename, cb) => cb(new Error('err')); + const prompt = 'prompt#'; + replStart({ input, output, eval: evalFn, prompt }); + input.end('s\n'); + if (!String(output.read()).includes('prompt#prompt#')) { + return; // All good, nothing to do here. + } + } + + // If it is, fix up the REPL's domain 'error' listener to not call displayPrompt() + const domain = (repl as any)._domain; + const domainErrorListeners = domain.listeners('error'); + const origListener = domainErrorListeners.find(fn => fn.name === 'debugDomainError'); + if (!origListener) { + throw new Error('Could not find REPL domain error listener'); + } + domain.removeListener('error', origListener); + domain.on('error', function(err) { + const origDisplayPrompt = repl.displayPrompt; + repl.displayPrompt = () => {}; + try { + origListener.call(this, err); + } finally { + repl.displayPrompt = origDisplayPrompt; + } + }); +} + /** * An instance of a `mongosh` REPL, without any of the actual I/O. */ @@ -113,6 +147,7 @@ class MongoshNodeRepl implements EvaluationListener { (err: Error) => Object.assign(new MongoshInternalError(err.message), { stack: err.stack }), ...this.nodeReplOptions }); + fixupReplForNodeBug38314(repl); const console = new Console({ stdout: this.output, diff --git a/packages/cli-repl/test/e2e.spec.ts b/packages/cli-repl/test/e2e.spec.ts index cd5c95cccb..755bfd0366 100644 --- a/packages/cli-repl/test/e2e.spec.ts +++ b/packages/cli-repl/test/e2e.spec.ts @@ -8,7 +8,7 @@ import { promises as fs, createReadStream } from 'fs'; import { promisify } from 'util'; import rimraf from 'rimraf'; import path from 'path'; -import { readReplLogfile, hasNodeBug38314 } from './repl-helpers'; +import { readReplLogfile } from './repl-helpers'; describe('e2e', function() { const testServer = startTestServer('shared'); @@ -558,8 +558,6 @@ describe('e2e', function() { let result; result = await shell.executeLine('require("a")'); expect(result).to.match(/Error: Cannot find module 'a'/); - // Wait for double prompt because of Node.js REPL bug - if (hasNodeBug38314()) await eventually(() => shell.assertContainsOutput('> > ')); result = await shell.executeLine('require("./a")'); expect(result).to.match(/^A$/m); result = await shell.executeLine('require("b")'); diff --git a/packages/cli-repl/test/repl-helpers.ts b/packages/cli-repl/test/repl-helpers.ts index 8d9bd0f5ff..be77d319cf 100644 --- a/packages/cli-repl/test/repl-helpers.ts +++ b/packages/cli-repl/test/repl-helpers.ts @@ -7,8 +7,6 @@ import chai, { expect } from 'chai'; import sinon from 'ts-sinon'; import sinonChai from 'sinon-chai'; import chaiAsPromised from 'chai-as-promised'; -import repl from 'repl'; -import { PassThrough } from 'stream'; import type { MongoshBus, MongoshBusEventsMap } from '@mongosh/types'; chai.use(sinonChai); @@ -78,17 +76,6 @@ async function readReplLogfile(logPath: string) { .map((line) => JSON.parse(line)); } -// https://github.com/nodejs/node/pull/38314 -function hasNodeBug38314() { - const input = new PassThrough(); - const output = new PassThrough(); - const evalFn = (code, ctx, filename, cb) => cb(new Error('err')); - const prompt = 'prompt#'; - repl.start({ input, output, eval: evalFn, prompt }); - input.end('s\n'); - return String(output.read()).includes('prompt#prompt#'); -} - export { expect, sinon, @@ -98,6 +85,5 @@ export { waitEval, waitCompletion, fakeTTYProps, - readReplLogfile, - hasNodeBug38314 + readReplLogfile }; From 657052b7fad5503ef3810a6fa052198b00ba40a0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 4 May 2021 15:08:57 +0200 Subject: [PATCH 2/4] fixup --- packages/cli-repl/src/mongosh-repl.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index 915762ae0d..c7183873c9 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -62,9 +62,9 @@ function fixupReplForNodeBug38314(repl: REPLServer): void { // Check whether bug is present: const input = new PassThrough(); const output = new PassThrough(); - const evalFn = (code, ctx, filename, cb) => cb(new Error('err')); + const evalFn = () => cb(new Error('err')); const prompt = 'prompt#'; - replStart({ input, output, eval: evalFn, prompt }); + replStart({ input, output, eval: evalFn as any, prompt }); input.end('s\n'); if (!String(output.read()).includes('prompt#prompt#')) { return; // All good, nothing to do here. @@ -74,12 +74,12 @@ function fixupReplForNodeBug38314(repl: REPLServer): void { // If it is, fix up the REPL's domain 'error' listener to not call displayPrompt() const domain = (repl as any)._domain; const domainErrorListeners = domain.listeners('error'); - const origListener = domainErrorListeners.find(fn => fn.name === 'debugDomainError'); + const origListener = domainErrorListeners.find((fn: any) => fn.name === 'debugDomainError'); if (!origListener) { throw new Error('Could not find REPL domain error listener'); } domain.removeListener('error', origListener); - domain.on('error', function(err) { + domain.on('error', function(this: any, err: Error) { const origDisplayPrompt = repl.displayPrompt; repl.displayPrompt = () => {}; try { From f5a8703aef55ee75e40f837b10540ab7b8aa1355 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 4 May 2021 15:21:39 +0200 Subject: [PATCH 3/4] fixup --- packages/cli-repl/src/mongosh-repl.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index c7183873c9..ff3637b57d 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -62,7 +62,7 @@ function fixupReplForNodeBug38314(repl: REPLServer): void { // Check whether bug is present: const input = new PassThrough(); const output = new PassThrough(); - const evalFn = () => cb(new Error('err')); + const evalFn = (code: any, ctx: any, filename: any, cb: any) => cb(new Error('err')); const prompt = 'prompt#'; replStart({ input, output, eval: evalFn as any, prompt }); input.end('s\n'); From 26a0de27bb285b1990d1b3891c118f0fe5d777f4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 6 May 2021 13:21:03 +0200 Subject: [PATCH 4/4] fixup: regression test for execution order problem --- packages/cli-repl/src/mongosh-repl.spec.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/cli-repl/src/mongosh-repl.spec.ts b/packages/cli-repl/src/mongosh-repl.spec.ts index 4bdc13a0c8..6a84906a47 100644 --- a/packages/cli-repl/src/mongosh-repl.spec.ts +++ b/packages/cli-repl/src/mongosh-repl.spec.ts @@ -226,6 +226,19 @@ describe('MongoshNodeRepl', () => { await waitEval(bus); expect(mongoshRepl.runtimeState().repl.listenerCount('SIGINT')).to.equal(1); }); + + it('does not run statements that should not run', async() => { + input.write(` + sleep(0); + throw new Error(); + if (false) + print ('!this should not run!'); + `); + for (let i = 0; i < 20; i++) { + await tick(); + } + expect(output).not.to.include('!this should not run!'); + }); }); context('with terminal: true', () => {