Skip to content

feat(node-runtime-worker-thread): Add new runtime that works in the worker thread COMPASS-4555 #533

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

Merged
merged 26 commits into from
Jan 12, 2021

Conversation

gribnoysup
Copy link
Collaborator

@gribnoysup gribnoysup commented Jan 7, 2021

This PR introduces new mongosh runtime that works in the worker thread and communicates with the main thread through an rpc that matches generic Runtime interface

To avoid making this PR even bigger we decided to split the work on the runtime in the multiple parts:

  • basic implementation that works, compiles and can provide some very basic ShellResults back to the main thread
  • "sync" code evaluation interruption
  • better serialization logic that will handle some special ShellResult cases like primitive values, cursors and errors
  • runtime messageBus integration for telemetry
  • integration with compass-shell

This PR only addresses the first step: basic implementation. It's mostly based on the PoC (#483) and follows tech design decisions pretty closely if you want additional context on some of the decisions made

Please don't be scared by the line count, its mostly due to the new package being set up and files like package-lock added to the repository. The PR is still pretty massive so sorry for that and I'm of course happy to go through all the changes together

Copy link
Collaborator

@mcasimir mcasimir left a comment

Choose a reason for hiding this comment

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

Maybe @addaleax wants to give a look too, but it looks amazing to me.

Copy link
Collaborator

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Nice work! :)

this.initWorkerPromise = this.initWorker();
}

private async initWorker() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, I’m a bit surprised that the linter is okay with leaving out return value descriptions… I thought it complained about that for other packages

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm, I can't see the rule enabled in our eslint config, and it's not a recommended one 🤔 Should we add it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No strong feelings about that, but I personally like being explicit about the signature 🤷‍♀️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I like the rule too and (as it's pretty obvious from this PR, haha) always forget to add explicit return types when the rule is not there 😄

@gribnoysup gribnoysup force-pushed the compass-4513-shell-runtime-worker-thread branch 2 times, most recently from 17c9c9c to c12e90b Compare January 11, 2021 17:05
@gribnoysup gribnoysup force-pushed the compass-4513-shell-runtime-worker-thread branch 2 times, most recently from 5683a88 to 45f26dc Compare January 12, 2021 14:04
@gribnoysup gribnoysup force-pushed the compass-4513-shell-runtime-worker-thread branch from 07d5e33 to d5aa34c Compare January 12, 2021 16:26
@gribnoysup gribnoysup merged commit 15d5101 into master Jan 12, 2021
@gribnoysup gribnoysup deleted the compass-4513-shell-runtime-worker-thread branch January 12, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants