-
Notifications
You must be signed in to change notification settings - Fork 77
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
Conversation
@@ -0,0 +1,62 @@ | |||
const loaderUtils = require('loader-utils'); |
There was a problem hiding this comment.
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!');"
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) }; | ||
} |
There was a problem hiding this comment.
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
…tween threads Co-authored-by: Anna Henningsen <[email protected]>
const newRuntime = new WorkerRuntime(url, options); | ||
|
||
// TODO: For testing purposes | ||
window.restart = newRuntime.restart.bind(newRuntime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
import { inspect } from './todo_this_needs_to_be_a_separate_package__inspect'; | ||
|
||
function serializeProfileResult({ result, count }) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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) }; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
…erilalize args when needed; Handle both message port and process when sending message
interruptWorker() { | ||
this.childProcess.kill('SIGINT'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error('Runtime is not initiated yet'); | ||
} | ||
|
||
const result = await runtime.evaluate(code); |
There was a problem hiding this comment.
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
mongosh/packages/cli-repl/src/async-repl.ts
Lines 40 to 53 in 4ac387f
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]>
37538dc
to
415f5d1
Compare
@@ -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", |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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? */); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄
Closing as this PR is now almost completely moved to real implementation in other PRs |
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.