Skip to content

Commit

Permalink
fix(cli-repl): do not run prompt and initial input in parallel MONGOS…
Browse files Browse the repository at this point in the history 
…H-1617 (#1856)

Users were getting incorrect behavior when:

1. A custom asynchronous prompt was set outside of the main REPL
   input, e.g. in a `.mongoshrc.js` file or a `--eval` script, and
2. The main REPL input was readable before we started evaluating
   the prompt.

because then, the first lines of input and the prompt may have ended
up executing in parallel. Setting the prompt before starting to process
input should resolve this issue.
  • Loading branch information
@addaleax
addaleax authored Mar 7, 2024
1 parent fcc77f0 commit d18ee6d
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 23 deletions.
96 changes: 76 additions & 20 deletions packages/cli-repl/src/mongosh-repl.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ import MongoshNodeRepl from './mongosh-repl';
import { parseAnyLogEntry } from '../../shell-api/src/log-entry';
import stripAnsi from 'strip-ansi';

function nonnull<T>(value: T | null | undefined): NonNullable<T> {
if (!value) throw new Error();
return value;
}

const delay = promisify(setTimeout);

const multilineCode = `(function() {
Expand Down Expand Up @@ -280,7 +285,7 @@ describe('MongoshNodeRepl', function () {
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(
expect(mongoshRepl.runtimeState().repl?.listenerCount('SIGINT')).to.equal(
1
);
});
Expand Down Expand Up @@ -548,7 +553,7 @@ describe('MongoshNodeRepl', function () {
await tick();
input.write('"bar" })\n');
await tick();
expect(mongoshRepl.runtimeState().repl.context.obj).to.deep.equal({
expect(mongoshRepl.runtimeState().context.obj).to.deep.equal({
foo: 'bar',
});
expect(output).not.to.include('obj = ({ foo: "bar" })');
Expand All @@ -571,7 +576,7 @@ describe('MongoshNodeRepl', function () {
await tick();
input.write('\u0004'); // Ctrl+D
await tick();
expect(mongoshRepl.runtimeState().repl.context.obj).to.deep.equal({
expect(mongoshRepl.runtimeState().context.obj).to.deep.equal({
foo: 'baz',
});
expect(output).not.to.include('obj = ({ foo: "baz" })');
Expand All @@ -596,7 +601,7 @@ describe('MongoshNodeRepl', function () {
await tick();
input.write('"bar" })\n');
await tick();
expect(mongoshRepl.runtimeState().repl.context.obj).to.deep.equal({
expect(mongoshRepl.runtimeState().context.obj).to.deep.equal({
foo: 'bar',
});
expect(output).not.to.include('obj = ({ foo: "bar" })');
Expand Down Expand Up @@ -638,19 +643,31 @@ describe('MongoshNodeRepl', function () {

it('does not crash if hitting enter and then up', async function () {
input.write('\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);
input.write(`${arrowUp}`);
await tick();
});

context('redaction', function () {
it('removes sensitive commands by default', async function () {
input.write('connect\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);
input.write('connection\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);
input.write('db.test.insert({ email: "[email protected]" })\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);

expect(getHistory()).to.deep.equal([
'db.test.insert({ email: "[email protected]" })',
Expand All @@ -662,11 +679,20 @@ describe('MongoshNodeRepl', function () {
input.write('config.set("redactHistory", "keep");\n');
await tick();
input.write('connect\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);
input.write('connection\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);
input.write('db.test.insert({ email: "[email protected]" })\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);

expect(getHistory()).to.deep.equal([
'db.test.insert({ email: "[email protected]" })',
Expand All @@ -680,11 +706,20 @@ describe('MongoshNodeRepl', function () {
input.write('config.set("redactHistory", "remove-redact");\n');
await tick();
input.write('connect\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);
input.write('connection\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);
input.write('db.test.insert({ email: "[email protected]" })\n');
await once(mongoshRepl.runtimeState().repl, 'flushHistory');
await once(
nonnull(mongoshRepl.runtimeState().repl),
'flushHistory'
);

expect(getHistory()).to.deep.equal([
'db.test.insert({ email: "<email>" })',
Expand Down Expand Up @@ -783,11 +818,9 @@ describe('MongoshNodeRepl', function () {

it('does not refresh the prompt if a window resize occurs while evaluating', async function () {
let resolveInProgress;
mongoshRepl.runtimeState().repl.context.inProgress = new Promise(
(resolve) => {
resolveInProgress = resolve;
}
);
mongoshRepl.runtimeState().context.inProgress = new Promise((resolve) => {
resolveInProgress = resolve;
});
input.write('inProgress\n');
await tick();

Expand Down Expand Up @@ -887,7 +920,7 @@ describe('MongoshNodeRepl', function () {
context('user prompts', function () {
beforeEach(function () {
// No boolean equivalent for 'passwordPrompt' in the API, so provide one:
mongoshRepl.runtimeState().repl.context.booleanPrompt = (question) => {
mongoshRepl.runtimeState().context.booleanPrompt = (question) => {
return Object.assign(mongoshRepl.onPrompt(question, 'yesno'), {
[Symbol.for('@@mongosh.syntheticPromise')]: true,
});
Expand Down Expand Up @@ -1283,6 +1316,29 @@ describe('MongoshNodeRepl', function () {
expect(output).to.contain('> ');
});
});

context('pre-specified user-provided prompt', function () {
it('does not attempt to run the prompt in parallel with initial input', async function () {
output = '';
const initialized = await mongoshRepl.initialize(serviceProvider);
await mongoshRepl.loadExternalCode(
`prompt = () => {
if (globalThis.isEvaluating) print('FAILED -- Parallel execution detected!');
globalThis.isEvaluating = true;
sleep(100);
globalThis.isEvaluating = false;
return '> ';
};`,
'test'
);
expect(mongoshRepl.runtimeState().context.prompt).to.be.a('function');
// Queue up input *before* starting the REPL itself
input.write('prompt()\n');
await mongoshRepl.startRepl(initialized);
await waitEval(bus);
expect(output).not.to.include('FAILED');
});
});
});

context('before the REPL starts', function () {
Expand Down
6 changes: 4 additions & 2 deletions packages/cli-repl/src/mongosh-repl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,11 +533,13 @@ class MongoshNodeRepl implements EvaluationListener {
'Cannot start REPL when not in REPL evaluation mode'
);
}
// Set up the prompt before consuming input so that we do not end up
// running the prompt function in parallel with actual input code.
repl.setPrompt(await this.getShellPrompt());
// Only start reading from the input *after* we set up everything, including
// instanceState.setCtx().
// instanceState.setCtx() and configuring the REPL prompt.
this.lineByLineInput.start();
this.input.resume();
repl.setPrompt(await this.getShellPrompt());
repl.displayPrompt();
}

Expand Down
39 changes: 39 additions & 0 deletions packages/e2e-tests/test/e2e.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,26 @@ describe('e2e', function () {
}
expect(buffer).to.include('"i": 99999');
});
it('handles custom prompt() function in conjunction with line-by-line input well', async function () {
// https://jira.mongodb.org/browse/MONGOSH-1617
shell = TestShell.start({
args: [
'--nodb',
'--shell',
'--eval',
'prompt = () => {sleep(1);return "x>"}',
],
});
// The number of newlines here matters
shell.writeInput(
'sleep(100);print([1,2,3,4,5,6,7,8,9,10].reduce(\n(a,b) => { return a*b; }, 1))\n\n\n\n',
{ end: true }
);
const exitCode = await shell.waitForExit();
expect(exitCode).to.equal(0);
shell.assertContainsOutput('3628800');
shell.assertNoErrors();
});
});

describe('set db', function () {
Expand Down Expand Up @@ -1035,6 +1055,25 @@ describe('e2e', function () {
shell.assertContainsOutput('admin;system.version;');
});
});

it('works fine with custom prompts', async function () {
// https://jira.mongodb.org/browse/MONGOSH-1617
shell = TestShell.start({
args: [
await testServer.connectionString(),
'--eval',
'prompt = () => db.stats().db',
'--shell',
],
});
shell.writeInput(
'[db.hello()].reduce(\n() => { return 11111*11111; },0)\n\n\n',
{ end: true }
);
await shell.waitForExit();
shell.assertContainsOutput('123454321');
shell.assertNoErrors();
});
});

describe('Node.js builtin APIs in the shell', function () {
Expand Down
3 changes: 2 additions & 1 deletion packages/e2e-tests/test/test-shell.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,9 @@ export class TestShell {
this._process.kill(signal);
}

writeInput(chars: string): void {
writeInput(chars: string, { end = false } = {}): void {
this._process.stdin.write(chars);
if (end) this._process.stdin.end();
}

writeInputLine(chars: string): void {
Expand Down

0 comments on commit d18ee6d

Please sign in to comment.