Skip to content

PoC: Compass shell worker runtime (COMPASS-4517) #483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from

Conversation

gribnoysup
Copy link
Collaborator

@gribnoysup gribnoysup commented Dec 9, 2020

PoC implementation of a mongosh runtime that is running in a separate thread created by spawning a child process done in the scope of COMPASS-4517 spike.

I'll leave some comments for the main points of interest in this implementation.

⚠️ This is just a reference implementation, feel free to adjust and change approaches as needed ⚠️

@@ -0,0 +1,62 @@
const loaderUtils = require('loader-utils');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file here is mostly copying whatever the workerize-loader is doing, removing all parts that are not really relevant to us: it allows to convert an imported to module to an entry chunk so all its dependencies are bundled with it and then returns a stringified version of the code:

// ./a.js
console.log('Hi!');

// ./b.js
import a from 'inline-entry-loader!./a.js'

console.log(typeof a === 'string'); // true
console.log(a) // "console.log('Hi!');"

Comment on lines +57 to +116
export function serializeResult(result) {
const { type, printable } = result;

if (typeof printable === 'string' && type !== null) {
return result;
// return <SimpleTypeOutput value={printable} raw />;
}

if (isPrimitiveOrFunction(printable)) {
return { printable: inspect(printable) };
// return <SimpleTypeOutput value={printable} />;
}

if (type === 'Help') {
return result;
// return <HelpOutput value={value} />;
}

if (type === 'ShowDatabasesResult') {
return result;
// return <ShowDbsOutput value={value} />;
}

if (type === 'StatsResult') {
return result;
// return <StatsResultOutput value={value} />;
}

if (type === 'ListCommandsResult') {
return { printable: inspect(printable) };
// return <SimpleTypeOutput value={printable} />;
}

if (type === 'ShowCollectionsResult') {
return result;
// return <ShowCollectionsOutput value={printable} />;
}

if (type === 'Cursor') {
return { type, printable: printable.map(inspect) };
// return <CursorOutput value={printable} />;
}

if (type === 'CursorIterationResult') {
return { type, printable: printable.map(inspect) };
// return <CursorIterationResultOutput value={printable} />;
}

if (type === 'ShowProfileResult') {
return { printable: serializeProfileResult(printable) };
// return <ShowProfileOutput value={printable} />;
}

if (isError(printable)) {
return { printable: serializeError(printable) };
// return <ErrorOutput value={printable} />;
}

return { printable: inspect(printable) };
}
Copy link
Collaborator Author

@gribnoysup gribnoysup Dec 9, 2020

Choose a reason for hiding this comment

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

Interestingly enough, not a lot of values had to be serialized before sending them back to the main thread which is great news for us

@gribnoysup gribnoysup changed the title PoC: Compass shell worker runtime PoC: Compass shell worker runtime (COMPASS-4517) Dec 10, 2020
const newRuntime = new WorkerRuntime(url, options);

// TODO: For testing purposes
window.restart = newRuntime.restart.bind(newRuntime);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you are running this branch locally, you can run a while(true){} in your shell and then manually call restart in the console, you'll see that the script just gracefully interrupts:

Kapture 2020-12-10 at 12 16 33


import { inspect } from './todo_this_needs_to_be_a_separate_package__inspect';

function serializeProfileResult({ result, count }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not so much about this PR. But how is it working now?

This one looks like a presentation concerns that (at least at the moment) would go in the React code, along to the other formatting elements.

Why is this not done in ShowProfileOutput?

Copy link
Collaborator Author

@gribnoysup gribnoysup Dec 10, 2020

Choose a reason for hiding this comment

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

It is copy-pasted from ShowProfileOutput and maybe is not needed here, I just wasn't sure if there might be any data in ShowProfileResult that runtime returns that will not survive serialization, so moved it here. This is definitely something to better examine when doing a final implementation

// return <ErrorOutput value={printable} />;
}

return { printable: inspect(printable) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably going to say something silly .. do you think printable here would be a bit overloaded with meanings? When consumed in the React side for some values will refer the printable property from the result of ShellEvaluator.eval and sometimes it would be the already rendered value from inspect, would it be expected / understandable enough for people working on that part of the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, yeah, the interface can be much more explicit for the final implementation, I agree

Comment on lines +112 to +114
interruptWorker() {
this.childProcess.kill('SIGINT');
}
Copy link
Collaborator Author

@gribnoysup gribnoysup Dec 16, 2020

Choose a reason for hiding this comment

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

Now we can cancel the evaluation and preserve the state 🎉

Kapture 2020-12-16 at 11 50 06

throw new Error('Runtime is not initiated yet');
}

const result = await runtime.evaluate(code);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can cancel sync op with SIGINT now, but async op is not handled in the worker. Something similar to async-repl implementation should be added there, but I think doesn't need to be implemented int he scope of the PoC

if (breakEvalOnSigint) {
// Handle SIGINT (Ctrl+C) that occurs while we are stuck in `await`
// by racing a listener for 'SIGINT' against the evalResult Promise.
// We remove all 'SIGINT' listeners and install our own.
sigintListener = (): void => {
// Reject with an exception similar to one thrown by Node.js
// itself if the `customEval` itself is interrupted.
reject(new Error('Asynchronous execution was interrupted by `SIGINT`'));
};
previousSigintListeners = repl.rawListeners('SIGINT');
repl.removeAllListeners('SIGINT');
repl.once('SIGINT', sigintListener);
}

…ting to fs; Use worker in child process to work around vm SIGINT issue

Co-authored-by: Anna Henningsen <[email protected]>
@gribnoysup gribnoysup force-pushed the compass-shell-worker-runtime branch from 37538dc to 415f5d1 Compare December 16, 2020 10:51
@@ -56,9 +56,11 @@
"@leafygreen-ui/icon-button": "^5.0.2",
"@mongosh/browser-repl": "^0.5.2",
"@mongosh/browser-runtime-electron": "^0.5.2",
"@mongosh/service-provider-core": "^0.5.2",
"@mongosh/service-provider-server": "^0.5.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you’ll need to rebase these :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah, for sure! I was actually thinking that now that we have figured out all the pieces of the implementation, it might be easier to start from current master and cherry-pick/copy pieces of this PR into smaller, logical parts that would be easier to review instead of a massive PR like that


async function init(uri, options, cliOptions) {
provider = await CliServiceProvider.connect(uri, options, cliOptions);
runtime = new ElectronRuntime(provider /** , TODO: messageBus? */);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think we’d need the messageBus for telemetry? But anything that vaguely looks like an EventEmitter should suffice here

throw new Error('Runtime is not initiated yet');
}

const result = await runtime.evaluate(code);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in order for the SIGINT to work reliably, we’d also need a dummy process.on('SIGINT', noop) listener in the main thread of the child process? Otherwise a mis-timed Ctrl+C would still bring the child process down, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, totally something to handle better than it is done here 😄

@gribnoysup
Copy link
Collaborator Author

Closing as this PR is now almost completely moved to real implementation in other PRs

@gribnoysup gribnoysup closed this Jan 26, 2021
@gribnoysup gribnoysup deleted the compass-shell-worker-runtime branch January 26, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge POC An idea or example
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants