From e207e04a6cafd4cff061973918a81fa37d3e33e7 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 1 Jul 2024 17:23:25 +0200 Subject: [PATCH 01/26] remove worker proxy --- .../src/child-process-evaluation-listener.ts | 68 ----- .../src/child-process-proxy.spec.ts | 84 ------ .../src/child-process-proxy.ts | 124 --------- ...-runtime.spec.ts => child-process.spec.ts} | 247 +----------------- .../{worker-runtime.ts => child-process.ts} | 84 +----- .../src/index.spec.ts | 37 +-- .../node-runtime-worker-thread/src/index.ts | 31 +-- .../src/spawn-child-from-source.spec.ts | 90 ------- .../src/spawn-child-from-source.ts | 102 -------- 9 files changed, 45 insertions(+), 822 deletions(-) delete mode 100644 packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts delete mode 100644 packages/node-runtime-worker-thread/src/child-process-proxy.spec.ts delete mode 100644 packages/node-runtime-worker-thread/src/child-process-proxy.ts rename packages/node-runtime-worker-thread/src/{worker-runtime.spec.ts => child-process.spec.ts} (66%) rename packages/node-runtime-worker-thread/src/{worker-runtime.ts => child-process.ts} (62%) delete mode 100644 packages/node-runtime-worker-thread/src/spawn-child-from-source.spec.ts delete mode 100644 packages/node-runtime-worker-thread/src/spawn-child-from-source.ts diff --git a/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts b/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts deleted file mode 100644 index 6ed6a7441..000000000 --- a/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts +++ /dev/null @@ -1,68 +0,0 @@ -import type { ChildProcess } from 'child_process'; -import type { Exposed } from './rpc'; -import { exposeAll, close } from './rpc'; -import type { WorkerRuntime } from './index'; -import { deserializeEvaluationResult } from './serializer'; -import type { RuntimeEvaluationListener } from '@mongosh/browser-runtime-core'; - -export class ChildProcessEvaluationListener { - exposedListener: Exposed< - Required< - Omit - > - >; - - constructor(workerRuntime: WorkerRuntime, childProcess: ChildProcess) { - this.exposedListener = exposeAll( - { - onPrompt(question, type) { - return ( - workerRuntime.evaluationListener?.onPrompt?.(question, type) ?? '' - ); - }, - onPrint(values) { - values = values.map(deserializeEvaluationResult); - return workerRuntime.evaluationListener?.onPrint?.(values); - }, - setConfig(key, value) { - return ( - workerRuntime.evaluationListener?.setConfig?.(key, value) ?? - Promise.resolve('ignored') - ); - }, - resetConfig(key) { - return ( - workerRuntime.evaluationListener?.resetConfig?.(key) ?? - Promise.resolve('ignored') - ); - }, - validateConfig(key, value) { - return ( - workerRuntime.evaluationListener?.validateConfig?.(key, value) ?? - Promise.resolve(null) - ); - }, - getConfig(key) { - return workerRuntime.evaluationListener?.getConfig?.(key); - }, - listConfigOptions() { - return workerRuntime.evaluationListener?.listConfigOptions?.(); - }, - onClearCommand() { - return workerRuntime.evaluationListener?.onClearCommand?.(); - }, - onExit(exitCode) { - return ( - workerRuntime.evaluationListener?.onExit?.(exitCode) ?? - (Promise.resolve() as Promise) - ); - }, - }, - childProcess - ); - } - - terminate() { - this.exposedListener[close](); - } -} diff --git a/packages/node-runtime-worker-thread/src/child-process-proxy.spec.ts b/packages/node-runtime-worker-thread/src/child-process-proxy.spec.ts deleted file mode 100644 index 7c32db268..000000000 --- a/packages/node-runtime-worker-thread/src/child-process-proxy.spec.ts +++ /dev/null @@ -1,84 +0,0 @@ -import path from 'path'; -import type { ChildProcess } from 'child_process'; -import { fork, spawn } from 'child_process'; -import type { Caller } from './rpc'; -import { cancel, createCaller } from './rpc'; -import { expect } from 'chai'; -import type { WorkerRuntime } from './worker-runtime'; -import { once } from 'events'; -import { promisify } from 'util'; -import { dummyOptions } from './index.spec'; - -const childProcessModulePath = path.resolve( - __dirname, - '..', - 'dist', - 'child-process-proxy.js' -); - -describe('child process worker proxy', function () { - let caller: Caller; - let childProcess: ChildProcess; - - afterEach(function () { - if (caller) { - caller[cancel](); - caller = null; - } - - if (childProcess) { - childProcess.disconnect(); - childProcess = null; - } - }); - - it('should start worker runtime and proxy calls', async function () { - childProcess = fork(childProcessModulePath); - caller = createCaller(['init', 'evaluate'], childProcess); - await caller.init('mongodb://nodb/', dummyOptions, { nodb: true }); - const result = await caller.evaluate('1 + 1'); - expect(result.printable).to.equal(2); - }); - - it('should exit on its own when the parent process disconnects', async function () { - const intermediateProcess = spawn( - process.execPath, - [ - '-e', - `require("child_process") - .fork(${JSON.stringify(childProcessModulePath)}) - .on("message", function(m) { console.log("message " + m + " from " + this.pid) })`, - ], - { stdio: ['pipe', 'pipe', 'inherit'] } - ); - - // Make sure the outer child process runs and has created the inner child process - const [message] = await once( - intermediateProcess.stdout.setEncoding('utf8'), - 'data' - ); - const match = message.trim().match(/^message ready from (?\d+)$/); - expect(match).to.not.equal(null); - - // Make sure the inner child process runs - const childPid = +match.groups.pid; - process.kill(childPid, 0); - - // Kill the intermediate process and wait for the inner child process to also close - intermediateProcess.kill('SIGTERM'); - let innerChildHasStoppedRunning = false; - for (let i = 0; i < 200; i++) { - try { - process.kill(childPid, 0); - } catch (err) { - if (err.code === 'ESRCH') { - innerChildHasStoppedRunning = true; - break; - } - throw err; - } - await promisify(setTimeout)(10); - } - expect(innerChildHasStoppedRunning).to.equal(true); - }); -}); diff --git a/packages/node-runtime-worker-thread/src/child-process-proxy.ts b/packages/node-runtime-worker-thread/src/child-process-proxy.ts deleted file mode 100644 index 633878869..000000000 --- a/packages/node-runtime-worker-thread/src/child-process-proxy.ts +++ /dev/null @@ -1,124 +0,0 @@ -/* istanbul ignore file */ -/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */ - -/** - * This proxy is needed as a workaround for the old electron verison "bug" where - * due to the electron runtime being a chromium, not just node (even with - * `ELECTRON_RUN_AS_NODE` enabled), SIGINT doesn't break code execution. This is - * fixed in the later versions of electron/node but we are still on the older - * one, we have to have this proxy in place - * - * @todo as soon as we update electron version in compass, we can get rid of - * this part of the worker runtime as it becomes redundant - * - * @see {@link https://github.com/nodejs/node/pull/36344} - */ -import { once } from 'events'; -import { SHARE_ENV, Worker } from 'worker_threads'; -import path from 'path'; -import { exposeAll, createCaller } from './rpc'; -import type { InterruptHandle } from 'interruptor'; -import { interrupt as nativeInterrupt } from 'interruptor'; - -const workerRuntimeSrcPath = - process.env.WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING || - path.resolve(__dirname, 'worker-runtime.js'); - -const workerProcess = new Worker(workerRuntimeSrcPath, { env: SHARE_ENV }); - -const workerReadyPromise: Promise = (async () => { - const waitForReadyMessage = async () => { - let msg: string; - while (([msg] = await once(workerProcess, 'message'))) { - if (msg === 'ready') return; - } - }; - - const waitForError = async () => { - const [err] = await once(workerProcess, 'error'); - if (err) { - err.message = `Worker thread failed to start with the following error: ${err.message}`; - throw err; - } - }; - - await Promise.race([waitForReadyMessage(), waitForError()]); -})(); - -// We expect the amount of listeners to be more than the default value of 10 but -// probably not more than ~25 (all exposed methods on -// ChildProcessEvaluationListener and ChildProcessMongoshBus + any concurrent -// in-flight calls on ChildProcessRuntime) at once -process.setMaxListeners(25); -workerProcess.setMaxListeners(25); - -let interruptHandle: InterruptHandle | null = null; - -const { interrupt } = createCaller(['interrupt'], workerProcess); - -const worker = Object.assign( - createCaller( - ['init', 'evaluate', 'getCompletions', 'getShellPrompt'], - workerProcess - ), - { - interrupt(): boolean { - if (interruptHandle) { - nativeInterrupt(interruptHandle); - return true; - } - - return interrupt(); - }, - } -); - -function waitForWorkerReadyProxy(fn: T): T { - return new Proxy(fn, { - async apply(target, thisArg, argumentsList) { - await workerReadyPromise; - return target.call(thisArg, ...Array.from(argumentsList)); - }, - }); -} - -// Every time parent process wants to request something from worker through -// proxy, we want to make sure worker process is ready -(Object.keys(worker) as (keyof typeof worker)[]).forEach((key) => { - worker[key] = waitForWorkerReadyProxy(worker[key]); -}); - -exposeAll(worker, process); - -const evaluationListener = Object.assign( - createCaller( - [ - 'onPrint', - 'onPrompt', - 'getConfig', - 'setConfig', - 'resetConfig', - 'validateConfig', - 'listConfigOptions', - 'onClearCommand', - 'onExit', - ], - process - ), - { - onRunInterruptible(handle: InterruptHandle | null) { - interruptHandle = handle; - }, - } -); - -exposeAll(evaluationListener, workerProcess); - -const messageBus = createCaller(['emit', 'on'], process); - -exposeAll(messageBus, workerProcess); - -process.once('disconnect', () => process.exit()); -process.nextTick(() => { - process.send?.('ready'); -}); diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/child-process.spec.ts similarity index 66% rename from packages/node-runtime-worker-thread/src/worker-runtime.spec.ts rename to packages/node-runtime-worker-thread/src/child-process.spec.ts index feb286204..47ebe6257 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/child-process.spec.ts @@ -1,18 +1,16 @@ import path from 'path'; import { once } from 'events'; -import { Worker } from 'worker_threads'; import chai, { expect } from 'chai'; import sinonChai from 'sinon-chai'; -import sinon from 'sinon'; import { EJSON, ObjectId } from 'bson'; import { startSharedTestServer } from '../../../testing/integration-testing-hooks'; import type { Caller, Exposed } from './rpc'; import { cancel, close, createCaller, exposeAll } from './rpc'; import { deserializeEvaluationResult } from './serializer'; -import type { WorkerRuntime } from './worker-runtime'; +import type { WorkerRuntime } from './child-process'; import type { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; -import { interrupt } from 'interruptor'; import { dummyOptions } from './index.spec'; +import { type ChildProcess, spawn } from 'child_process'; chai.use(sinonChai); @@ -21,20 +19,22 @@ const workerThreadModule = path.resolve( __dirname, '..', 'dist', - 'worker-runtime.js' + 'child-process.js' ); function sleep(ms: number) { return new Promise((resolve) => setTimeout(resolve, ms)); } -describe('worker', function () { - let worker: Worker; +describe('child-process', function () { + let childProcess: ChildProcess; let caller: Caller; beforeEach(async function () { - worker = new Worker(workerThreadModule); - await once(worker, 'message'); + childProcess = spawn(process.execPath, [workerThreadModule], { + stdio: ['inherit', 'inherit', 'pipe', 'ipc'], + }); + await once(childProcess, 'message'); caller = createCaller( [ @@ -45,7 +45,7 @@ describe('worker', function () { 'setEvaluationListener', 'interrupt', ], - worker + childProcess ); const origEvaluate = caller.evaluate; caller.evaluate = (code: string): Promise & { cancel(): void } => { @@ -55,20 +55,9 @@ describe('worker', function () { }; }); - afterEach(async function () { - if (worker) { - // There is a Node.js bug that causes worker process to still be ref-ed - // after termination. To work around that, we are unrefing worker manually - // *immediately* after terminate method is called even though it should - // not be necessary. If this is not done in rare cases our test suite can - // get stuck. Even though the issue is fixed we would still need to keep - // this workaround for compat reasons. - // - // See: https://github.com/nodejs/node/pull/37319 - const terminationPromise = worker.terminate(); - worker.unref(); - await terminationPromise; - worker = null; + afterEach(function () { + if (childProcess) { + childProcess.kill(); } if (caller) { @@ -351,7 +340,7 @@ describe('worker', function () { getConfig() {}, validateConfig() {}, }, - worker + childProcess ); const { init, evaluate } = caller; @@ -491,214 +480,6 @@ describe('worker', function () { }); }); - describe('evaluationListener', function () { - const spySandbox = sinon.createSandbox(); - - const createSpiedEvaluationListener = () => { - const evalListener = { - onPrint() {}, - onPrompt() { - return '123'; - }, - getConfig() {}, - setConfig() {}, - resetConfig() {}, - validateConfig() {}, - listConfigOptions() { - return ['displayBatchSize']; - }, - onRunInterruptible() {}, - }; - - spySandbox.spy(evalListener, 'onPrint'); - spySandbox.spy(evalListener, 'onPrompt'); - spySandbox.spy(evalListener, 'getConfig'); - spySandbox.spy(evalListener, 'setConfig'); - spySandbox.spy(evalListener, 'resetConfig'); - spySandbox.spy(evalListener, 'validateConfig'); - spySandbox.spy(evalListener, 'listConfigOptions'); - spySandbox.spy(evalListener, 'onRunInterruptible'); - - return evalListener; - }; - - let exposed: Exposed; - - afterEach(function () { - if (exposed) { - exposed[close](); - exposed = null; - } - - spySandbox.restore(); - }); - - describe('onPrint', function () { - it('should be called when shell evaluates `print`', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - await evaluate('print("Hi!")'); - - expect(evalListener.onPrint).to.have.been.calledWith([ - { printable: 'Hi!', source: undefined, type: null }, - ]); - }); - - it('should correctly serialize bson objects', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - await evaluate('print(new ObjectId("62a209b0c7dc31e23ab9da45"))'); - - expect(evalListener.onPrint).to.have.been.calledWith([ - { - printable: "ObjectId('62a209b0c7dc31e23ab9da45')", - source: undefined, - type: 'InspectResult', - }, - ]); - }); - }); - - describe('onPrompt', function () { - it('should be called when shell evaluates `passwordPrompt`', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - const password = await evaluate('passwordPrompt()'); - - expect(evalListener.onPrompt).to.have.been.called; - expect(password.printable).to.equal('123'); - }); - }); - - describe('getConfig', function () { - it('should be called when shell evaluates `config.get()`', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - - await evaluate('config.get("key")'); - expect(evalListener.getConfig).to.have.been.calledWith('key'); - }); - }); - - describe('setConfig', function () { - it('should be called when shell evaluates `config.set()`', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - - await evaluate('config.set("displayBatchSize", 200)'); - expect(evalListener.validateConfig).to.have.been.calledWith( - 'displayBatchSize', - 200 - ); - expect(evalListener.setConfig).to.have.been.calledWith( - 'displayBatchSize', - 200 - ); - }); - }); - - describe('resetConfig', function () { - it('should be called when shell evaluates `config.reset()`', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - - await evaluate('config.reset("displayBatchSize")'); - expect(evalListener.resetConfig).to.have.been.calledWith( - 'displayBatchSize' - ); - }); - }); - - describe('listConfigOptions', function () { - it('should be called when shell evaluates `config[asPrintable]`', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - - await evaluate(` - var JSSymbol = Object.getOwnPropertySymbols(Array.prototype)[0].constructor; - config[JSSymbol.for("@@mongosh.asPrintable")]()`); - expect(evalListener.listConfigOptions).to.have.been.calledWith(); - }); - }); - - describe('onRunInterruptible', function () { - it('should call callback when interruptible evaluation starts and ends', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - await evaluate('1+1'); - - const [firstCall, secondCall] = ( - evalListener.onRunInterruptible as sinon.SinonSpy - ).args; - - expect(firstCall[0]).to.have.property('__id'); - expect(secondCall[0]).to.equal(null); - }); - - it('should return a handle that allows to interrupt the evaluation', async function () { - const { init, evaluate } = caller; - const evalListener = createSpiedEvaluationListener(); - - exposed = exposeAll(evalListener, worker); - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - - let err: Error; - - try { - await Promise.all([ - evaluate('while(true){}'), - (async () => { - await sleep(50); - const handle = (evalListener.onRunInterruptible as sinon.SinonSpy) - .args[0][0]; - interrupt(handle); - })(), - ]); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/Script execution was interrupted/); - }); - }); - }); - describe('interrupt', function () { it('should interrupt in-flight async tasks', async function () { const { init, evaluate, interrupt } = caller; diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.ts b/packages/node-runtime-worker-thread/src/child-process.ts similarity index 62% rename from packages/node-runtime-worker-thread/src/worker-runtime.ts rename to packages/node-runtime-worker-thread/src/child-process.ts index 9e12acfcb..d8b48f9f4 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.ts +++ b/packages/node-runtime-worker-thread/src/child-process.ts @@ -1,37 +1,28 @@ -/* istanbul ignore file */ -/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */ - -import { parentPort, isMainThread } from 'worker_threads'; import type { Completion, Runtime, - RuntimeEvaluationListener, RuntimeEvaluationResult, } from '@mongosh/browser-runtime-core'; import { ElectronRuntime } from '@mongosh/browser-runtime-electron'; import type { ServiceProvider } from '@mongosh/service-provider-core'; import { CompassServiceProvider } from '@mongosh/service-provider-server'; -import { exposeAll, createCaller } from './rpc'; +import { exposeAll } from './rpc'; import { serializeEvaluationResult, deserializeConnectOptions, } from './serializer'; -import type { MongoshBus } from '@mongosh/types'; import type { UNLOCKED } from './lock'; import { Lock } from './lock'; import type { InterruptHandle } from 'interruptor'; -import { runInterruptible } from 'interruptor'; +import { runInterruptible, interrupt as nativeInterrupt } from 'interruptor'; type DevtoolsConnectOptions = Parameters< (typeof CompassServiceProvider)['connect'] >[1]; -if (!parentPort || isMainThread) { - throw new Error('Worker runtime can be used only in a worker thread'); -} - let runtime: Runtime | null = null; let provider: ServiceProvider | null = null; +let interruptHandle: InterruptHandle | null = null; const evaluationLock = new Lock(); @@ -45,49 +36,6 @@ function ensureRuntime(methodName: string): Runtime { return runtime; } -export type WorkerRuntimeEvaluationListener = RuntimeEvaluationListener & { - onRunInterruptible(handle: InterruptHandle | null): void; -}; - -const evaluationListener = createCaller( - [ - 'onPrint', - 'onPrompt', - 'getConfig', - 'setConfig', - 'resetConfig', - 'validateConfig', - 'listConfigOptions', - 'onClearCommand', - 'onExit', - 'onRunInterruptible', - ], - parentPort, - { - onPrint: function ( - results: RuntimeEvaluationResult[] - ): RuntimeEvaluationResult[][] { - // We're transforming an args array, so we have to return an array of - // args. onPrint only takes one arg which is an array of - // RuntimeEvaluationResult so in this case it will just return a - // single-element array that itself is an array. - return [results.map(serializeEvaluationResult)]; - }, - } -); - -const messageBus: MongoshBus = Object.assign( - createCaller(['emit'], parentPort), - { - on() { - throw new Error("Can't call `on` method on worker runtime MongoshBus"); - }, - once() { - throw new Error("Can't call `once` method on worker runtime MongoshBus"); - }, - } -); - export type WorkerRuntime = Runtime & { init( uri: string, @@ -116,10 +64,9 @@ const workerRuntime: WorkerRuntime = { uri, deserializeConnectOptions(driverOptions), cliOptions, - messageBus + process ); - runtime = new ElectronRuntime(provider as ServiceProvider, messageBus); - runtime.setEvaluationListener(evaluationListener); + runtime = new ElectronRuntime(provider as ServiceProvider, process); }, async evaluate(code: string) { @@ -136,16 +83,14 @@ const workerRuntime: WorkerRuntime = { try { evaluationPromise = runInterruptible((handle) => { try { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - evaluationListener.onRunInterruptible(handle); + interruptHandle = handle; return ensureRuntime('evaluate').evaluate(code); } finally { interrupted = false; } }); } finally { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - evaluationListener.onRunInterruptible(null); + interruptHandle = null; if (interrupted) { // If we were interrupted, we can't know which newly require()ed modules @@ -193,20 +138,15 @@ const workerRuntime: WorkerRuntime = { }, interrupt() { + if (interruptHandle) { + nativeInterrupt(interruptHandle); + } return evaluationLock.unlock(); }, }; -// We expect the amount of listeners to be more than the default value of 10 but -// probably not more than ~25 (all exposed methods on -// ChildProcessEvaluationListener and ChildProcessMongoshBus + any concurrent -// in-flight calls on ChildProcessRuntime) at once -parentPort.setMaxListeners(25); - -exposeAll(workerRuntime, parentPort); +exposeAll(workerRuntime, process); process.nextTick(() => { - if (parentPort) { - parentPort.postMessage('ready'); - } + process.send?.('ready'); }); diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index 7037a812f..c4ccffd39 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -24,7 +24,7 @@ function sleep(ms: number) { } describe('WorkerRuntime', function () { - let runtime: WorkerRuntime; + let runtime: WorkerRuntime | null = null; afterEach(async function () { if (runtime) { @@ -67,33 +67,6 @@ describe('WorkerRuntime', function () { .to.have.property('message') .match(/Child process failed to start/); }); - - it('should return init error if worker in child process failed to spawn', async function () { - runtime = new WorkerRuntime( - 'mongodb://nodb/', - dummyOptions, - { nodb: true }, - { - env: { - WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING: - brokenScript, - }, - } - ); - - let err; - - try { - await runtime.evaluate('1+1'); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/Worker thread failed to start/); - }); }); describe('evaluate', function () { @@ -219,7 +192,7 @@ describe('WorkerRuntime', function () { }); }); - describe('setEvaluationListener', function () { + describe.skip('setEvaluationListener', function () { it('allows to set evaluation listener for runtime', async function () { const evalListener = { onPrompt: sinon.spy(() => 'password123'), @@ -238,7 +211,7 @@ describe('WorkerRuntime', function () { }); }); - describe('eventEmitter', function () { + describe.skip('eventEmitter', function () { const testServer = startSharedTestServer(); it('should propagate emitted events from worker', async function () { @@ -348,7 +321,7 @@ describe('WorkerRuntime', function () { .match(/Async script execution was interrupted/); }); - it('should interrupt in-flight synchronous tasks', async function () { + it.skip('should interrupt in-flight synchronous tasks', async function () { runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { nodb: true, }); @@ -375,7 +348,7 @@ describe('WorkerRuntime', function () { .match(/Script execution was interrupted/); }); - it('should allow to evaluate again after interruption', async function () { + it.skip('should allow to evaluate again after interruption', async function () { runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { nodb: true, }); diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index 818ad57f5..7c6646cb6 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -11,11 +11,9 @@ import type { import type { MongoshBus } from '@mongosh/types'; import path from 'path'; import { EventEmitter, once } from 'events'; -import { kill } from './spawn-child-from-source'; import type { Caller } from './rpc'; import { createCaller, cancel } from './rpc'; -import { ChildProcessEvaluationListener } from './child-process-evaluation-listener'; -import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './worker-runtime'; +import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './child-process'; import { deserializeEvaluationResult, serializeConnectOptions, @@ -28,6 +26,16 @@ type DevtoolsConnectOptions = Parameters< >[1]; type ChildProcessRuntime = Caller; +async function kill( + childProcess: ChildProcess, + code: NodeJS.Signals | number = 'SIGTERM' +) { + childProcess.kill(code); + if (childProcess.exitCode === null && childProcess.signalCode === null) { + await once(childProcess, 'exit'); + } +} + function parseStderrToError(str: string): Error | null { const [, errorMessageWithStack] = str .split(/^\s*\^\s*$/m) @@ -67,8 +75,6 @@ class WorkerRuntime implements Runtime { private childProcessMongoshBus!: ChildProcessMongoshBus; - private childProcessEvaluationListener!: ChildProcessEvaluationListener; - private childProcess!: ChildProcess; private childProcessRuntime!: ChildProcessRuntime; @@ -78,7 +84,7 @@ class WorkerRuntime implements Runtime { private childProcessProxySrcPath: string = process.env .CHILD_PROCESS_PROXY_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING || - path.resolve(__dirname, 'child-process-proxy.js'); + path.resolve(__dirname, 'child-process.js'); constructor( uri: string, @@ -141,8 +147,8 @@ class WorkerRuntime implements Runtime { // We expect the amount of listeners to be more than the default value of 10 // but probably not more than ~25 (all exposed methods on - // ChildProcessEvaluationListener and ChildProcessMongoshBus + any - // concurrent in-flight calls on ChildProcessRuntime) at once + // ChildProcessMongoshBus + any concurrent in-flight calls + // on ChildProcessRuntime) at once this.childProcess.setMaxListeners(25); this.childProcessRuntime = createCaller( @@ -157,11 +163,6 @@ class WorkerRuntime implements Runtime { this.childProcess ); - this.childProcessEvaluationListener = new ChildProcessEvaluationListener( - this, - this.childProcess - ); - this.childProcessMongoshBus = new ChildProcessMongoshBus( this.eventEmitter, this.childProcess @@ -211,10 +212,6 @@ class WorkerRuntime implements Runtime { this.childProcessRuntime[cancel](); } - if (this.childProcessEvaluationListener) { - this.childProcessEvaluationListener.terminate(); - } - if (this.childProcessMongoshBus) { this.childProcessMongoshBus.terminate(); } diff --git a/packages/node-runtime-worker-thread/src/spawn-child-from-source.spec.ts b/packages/node-runtime-worker-thread/src/spawn-child-from-source.spec.ts deleted file mode 100644 index 719cea0c1..000000000 --- a/packages/node-runtime-worker-thread/src/spawn-child-from-source.spec.ts +++ /dev/null @@ -1,90 +0,0 @@ -import { expect } from 'chai'; -import type { ChildProcess } from 'child_process'; -import childProcess from 'child_process'; -import { once } from 'events'; -import spawnChildFromSource, { kill } from './spawn-child-from-source'; - -describe('spawnChildFromSource', function () { - let spawned: ChildProcess; - - afterEach(async function () { - if (spawned) { - await kill(spawned, 'SIGKILL'); - spawned = null; - } - }); - - it('should throw if stdin is missing', async function () { - let err: Error; - - try { - spawned = await spawnChildFromSource('console.log("Hi")', { - // Making istanbul happy by passing stuff that's not allowed - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error - stdio: 'ignore', - }); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/missing stdin/); - }); - - it('should resolve with a child process', async function () { - spawned = await spawnChildFromSource(''); - expect(spawned).to.be.instanceof((childProcess as any).ChildProcess); - }); - - it('should spawn a process with an ipc channel open', async function () { - spawned = await spawnChildFromSource( - 'process.on("message", (data) => process.send(data))' - ); - spawned.send('Hi!'); - const [message] = await once(spawned, 'message'); - expect(message).to.equal('Hi!'); - }); - - it('should fail if process exited before successfully starting', async function () { - let err: Error; - - try { - spawned = await spawnChildFromSource( - 'throw new Error("Whoops!")', - {}, - undefined, - 'ignore', - 'ignore' - ); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err.message).to.match( - /Child process exited with error before starting/ - ); - }); - - it('should fail if a timeout exceeded before the process is "ready"', async function () { - let err: Error; - - try { - spawned = await spawnChildFromSource( - 'let i = 0; while(++i < 10000000000){};', - {}, - 10 - ); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err.message).to.match( - /Timed out while waiting for child process to start/ - ); - }); -}); diff --git a/packages/node-runtime-worker-thread/src/spawn-child-from-source.ts b/packages/node-runtime-worker-thread/src/spawn-child-from-source.ts deleted file mode 100644 index 5d35128f5..000000000 --- a/packages/node-runtime-worker-thread/src/spawn-child-from-source.ts +++ /dev/null @@ -1,102 +0,0 @@ -import type { - ChildProcess, - Serializable, - SpawnOptions, - StdioNull, - StdioPipe, -} from 'child_process'; -import { spawn } from 'child_process'; -import { once } from 'events'; - -export async function kill( - childProcess: ChildProcess, - code: NodeJS.Signals | number = 'SIGTERM' -) { - childProcess.kill(code); - if (childProcess.exitCode === null && childProcess.signalCode === null) { - await once(childProcess, 'exit'); - } -} - -export default function spawnChildFromSource( - src: string, - spawnOptions: Omit = {}, - timeoutMs?: number, - _stdout: StdioNull | StdioPipe = 'inherit', - _stderr: StdioNull | StdioPipe = 'inherit' -): Promise { - return new Promise((resolve, reject) => { - const readyToken = Date.now().toString(32); - - const childProcess = spawn(process.execPath, { - stdio: ['pipe', _stdout, _stderr, 'ipc'], - ...spawnOptions, - }); - - if (!childProcess.stdin) { - kill(childProcess) - .then(() => { - reject( - new Error("Can't write src to the spawned process, missing stdin") - ); - }) - .catch((err: any) => { - reject(err); - }); - return; - } - - // eslint-disable-next-line prefer-const - let timeoutId: NodeJS.Timeout | null; - - function cleanupListeners() { - if (timeoutId) { - clearTimeout(timeoutId); - } - if (childProcess.stdin) { - childProcess.stdin.off('error', onWriteError); - } - childProcess.off('message', onMessage); - childProcess.off('exit', onExit); - } - - function onExit(exitCode: number | null) { - if (exitCode && exitCode > 0) { - cleanupListeners(); - reject(new Error('Child process exited with error before starting')); - } - } - - /* really hard to reproduce in tests and coverage is not happy */ - /* istanbul ignore next */ - async function onWriteError(error: Error) { - cleanupListeners(); - await kill(childProcess); - reject(error); - } - - async function onTimeout() { - cleanupListeners(); - await kill(childProcess); - reject(new Error('Timed out while waiting for child process to start')); - } - - function onMessage(data: Serializable) { - if (data === readyToken) { - cleanupListeners(); - resolve(childProcess); - } - } - - childProcess.on('message', onMessage); - childProcess.on('exit', onExit); - childProcess.stdin.on('error', onWriteError); - - childProcess.stdin.write(src); - childProcess.stdin.write(`;process.send(${JSON.stringify(readyToken)})`); - childProcess.stdin.end(); - - timeoutId = - timeoutMs !== undefined ? setTimeout(onTimeout, timeoutMs) : null; - }); -} From 2850bcfe12d190790c27eebba53d9e818669da63 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 12:09:49 +0200 Subject: [PATCH 02/26] break on interrupt - sync and async --- .../src/child-process.spec.ts | 32 ------- .../src/child-process.ts | 89 ++++++++----------- .../src/index.spec.ts | 4 +- .../node-runtime-worker-thread/src/index.ts | 2 +- 4 files changed, 42 insertions(+), 85 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/child-process.spec.ts b/packages/node-runtime-worker-thread/src/child-process.spec.ts index 47ebe6257..74823c454 100644 --- a/packages/node-runtime-worker-thread/src/child-process.spec.ts +++ b/packages/node-runtime-worker-thread/src/child-process.spec.ts @@ -22,10 +22,6 @@ const workerThreadModule = path.resolve( 'child-process.js' ); -function sleep(ms: number) { - return new Promise((resolve) => setTimeout(resolve, ms)); -} - describe('child-process', function () { let childProcess: ChildProcess; let caller: Caller; @@ -43,7 +39,6 @@ describe('child-process', function () { 'getCompletions', 'getShellPrompt', 'setEvaluationListener', - 'interrupt', ], childProcess ); @@ -479,31 +474,4 @@ describe('child-process', function () { }); }); }); - - describe('interrupt', function () { - it('should interrupt in-flight async tasks', async function () { - const { init, evaluate, interrupt } = caller; - - await init('mongodb://nodb/', dummyOptions, { nodb: true }); - - let err: Error; - - try { - await Promise.all([ - evaluate('sleep(100000)'), - (async () => { - await sleep(10); - await interrupt(); - })(), - ]); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/Async script execution was interrupted/); - }); - }); }); diff --git a/packages/node-runtime-worker-thread/src/child-process.ts b/packages/node-runtime-worker-thread/src/child-process.ts index d8b48f9f4..12af45e80 100644 --- a/packages/node-runtime-worker-thread/src/child-process.ts +++ b/packages/node-runtime-worker-thread/src/child-process.ts @@ -13,16 +13,14 @@ import { } from './serializer'; import type { UNLOCKED } from './lock'; import { Lock } from './lock'; -import type { InterruptHandle } from 'interruptor'; -import { runInterruptible, interrupt as nativeInterrupt } from 'interruptor'; type DevtoolsConnectOptions = Parameters< (typeof CompassServiceProvider)['connect'] >[1]; +type EvaluationResult = void | RuntimeEvaluationResult | UNLOCKED; let runtime: Runtime | null = null; let provider: ServiceProvider | null = null; -let interruptHandle: InterruptHandle | null = null; const evaluationLock = new Lock(); @@ -42,10 +40,34 @@ export type WorkerRuntime = Runtime & { driverOptions?: DevtoolsConnectOptions, cliOptions?: { nodb?: boolean } ): Promise; - - interrupt(): boolean; }; +// If we were interrupted, we can't know which newly require()ed modules +// have successfully been loaded, so we restore everything to its +// previous state. +function clearRequireCache(previousRequireCache: string[]) { + for (const key of Object.keys(require.cache)) { + if (!previousRequireCache.includes(key)) { + delete require.cache[key]; + } + } +} + +function throwIfInterrupted( + result: EvaluationResult, + previousRequireCache: string[] +): asserts result is RuntimeEvaluationResult { + if (evaluationLock.isUnlockToken(result)) { + clearRequireCache(previousRequireCache); + throw new Error('Async script execution was interrupted'); + } + + if (typeof result === 'undefined') { + clearRequireCache(previousRequireCache); + throw new Error('Script execution was interrupted'); + } +} + const workerRuntime: WorkerRuntime = { async init( uri: string, @@ -76,50 +98,18 @@ const workerRuntime: WorkerRuntime = { ); } - let interrupted = true; - let evaluationPromise: void | Promise; const previousRequireCache = Object.keys(require.cache); + let result: EvaluationResult; try { - evaluationPromise = runInterruptible((handle) => { - try { - interruptHandle = handle; - return ensureRuntime('evaluate').evaluate(code); - } finally { - interrupted = false; - } - }); - } finally { - interruptHandle = null; - - if (interrupted) { - // If we were interrupted, we can't know which newly require()ed modules - // have successfully been loaded, so we restore everything to its - // previous state. - for (const key of Object.keys(require.cache)) { - if (!previousRequireCache.includes(key)) { - delete require.cache[key]; - } - } - } - } - - let result: void | RuntimeEvaluationResult | UNLOCKED; - - try { - result = await Promise.race([evaluationPromise, evaluationLock.lock()]); + result = await Promise.race([ + ensureRuntime('evaluate').evaluate(code), + evaluationLock.lock(), + ]); } finally { evaluationLock.unlock(); } - - if (evaluationLock.isUnlockToken(result)) { - throw new Error('Async script execution was interrupted'); - } - - if (typeof result === 'undefined' || interrupted === true) { - throw new Error('Script execution was interrupted'); - } - + throwIfInterrupted(result, previousRequireCache); return serializeEvaluationResult(result); }, @@ -136,17 +126,16 @@ const workerRuntime: WorkerRuntime = { 'Evaluation listener can not be directly set on the worker runtime' ); }, - - interrupt() { - if (interruptHandle) { - nativeInterrupt(interruptHandle); - } - return evaluationLock.unlock(); - }, }; exposeAll(workerRuntime, process); +// For async tasks, then an interrupt is received, we need to unlock the +// evaluation lock so that it resolves and workerRuntime.evaluate throws. +process.on('SIGINT', () => { + evaluationLock.unlock(); +}); + process.nextTick(() => { process.send?.('ready'); }); diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index c4ccffd39..77a408de3 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -321,7 +321,7 @@ describe('WorkerRuntime', function () { .match(/Async script execution was interrupted/); }); - it.skip('should interrupt in-flight synchronous tasks', async function () { + it('should interrupt in-flight synchronous tasks', async function () { runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { nodb: true, }); @@ -348,7 +348,7 @@ describe('WorkerRuntime', function () { .match(/Script execution was interrupted/); }); - it.skip('should allow to evaluate again after interruption', async function () { + it('should allow to evaluate again after interruption', async function () { runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { nodb: true, }); diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index 7c6646cb6..d93f38d5b 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -219,7 +219,7 @@ class WorkerRuntime implements Runtime { async interrupt() { await this.initWorkerPromise; - return this.childProcessRuntime.interrupt(); + this.childProcess.kill('SIGINT'); } async waitForRuntimeToBeReady() { From 32ee3f8914b9798e6dbba0a546c965bf398f9f0b Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 12:35:33 +0200 Subject: [PATCH 03/26] handle listeners --- .../src/child-process-evaluation-listener.ts | 68 +++++++++++++++++++ .../src/child-process.ts | 30 +++++++- .../src/index.spec.ts | 2 +- .../node-runtime-worker-thread/src/index.ts | 16 ++++- 4 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts diff --git a/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts b/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts new file mode 100644 index 000000000..6ed6a7441 --- /dev/null +++ b/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts @@ -0,0 +1,68 @@ +import type { ChildProcess } from 'child_process'; +import type { Exposed } from './rpc'; +import { exposeAll, close } from './rpc'; +import type { WorkerRuntime } from './index'; +import { deserializeEvaluationResult } from './serializer'; +import type { RuntimeEvaluationListener } from '@mongosh/browser-runtime-core'; + +export class ChildProcessEvaluationListener { + exposedListener: Exposed< + Required< + Omit + > + >; + + constructor(workerRuntime: WorkerRuntime, childProcess: ChildProcess) { + this.exposedListener = exposeAll( + { + onPrompt(question, type) { + return ( + workerRuntime.evaluationListener?.onPrompt?.(question, type) ?? '' + ); + }, + onPrint(values) { + values = values.map(deserializeEvaluationResult); + return workerRuntime.evaluationListener?.onPrint?.(values); + }, + setConfig(key, value) { + return ( + workerRuntime.evaluationListener?.setConfig?.(key, value) ?? + Promise.resolve('ignored') + ); + }, + resetConfig(key) { + return ( + workerRuntime.evaluationListener?.resetConfig?.(key) ?? + Promise.resolve('ignored') + ); + }, + validateConfig(key, value) { + return ( + workerRuntime.evaluationListener?.validateConfig?.(key, value) ?? + Promise.resolve(null) + ); + }, + getConfig(key) { + return workerRuntime.evaluationListener?.getConfig?.(key); + }, + listConfigOptions() { + return workerRuntime.evaluationListener?.listConfigOptions?.(); + }, + onClearCommand() { + return workerRuntime.evaluationListener?.onClearCommand?.(); + }, + onExit(exitCode) { + return ( + workerRuntime.evaluationListener?.onExit?.(exitCode) ?? + (Promise.resolve() as Promise) + ); + }, + }, + childProcess + ); + } + + terminate() { + this.exposedListener[close](); + } +} diff --git a/packages/node-runtime-worker-thread/src/child-process.ts b/packages/node-runtime-worker-thread/src/child-process.ts index 12af45e80..d63460b62 100644 --- a/packages/node-runtime-worker-thread/src/child-process.ts +++ b/packages/node-runtime-worker-thread/src/child-process.ts @@ -1,12 +1,13 @@ import type { Completion, Runtime, + RuntimeEvaluationListener, RuntimeEvaluationResult, } from '@mongosh/browser-runtime-core'; import { ElectronRuntime } from '@mongosh/browser-runtime-electron'; import type { ServiceProvider } from '@mongosh/service-provider-core'; import { CompassServiceProvider } from '@mongosh/service-provider-server'; -import { exposeAll } from './rpc'; +import { createCaller, exposeAll } from './rpc'; import { serializeEvaluationResult, deserializeConnectOptions, @@ -34,6 +35,32 @@ function ensureRuntime(methodName: string): Runtime { return runtime; } +const evaluationListener = createCaller( + [ + 'onPrint', + 'onPrompt', + 'getConfig', + 'setConfig', + 'resetConfig', + 'validateConfig', + 'listConfigOptions', + 'onClearCommand', + 'onExit', + ], + process, + { + onPrint: function ( + results: RuntimeEvaluationResult[] + ): RuntimeEvaluationResult[][] { + // We're transforming an args array, so we have to return an array of + // args. onPrint only takes one arg which is an array of + // RuntimeEvaluationResult so in this case it will just return a + // single-element array that itself is an array. + return [results.map(serializeEvaluationResult)]; + }, + } +); + export type WorkerRuntime = Runtime & { init( uri: string, @@ -89,6 +116,7 @@ const workerRuntime: WorkerRuntime = { process ); runtime = new ElectronRuntime(provider as ServiceProvider, process); + runtime.setEvaluationListener(evaluationListener); }, async evaluate(code: string) { diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index 77a408de3..bcd9e48cc 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -192,7 +192,7 @@ describe('WorkerRuntime', function () { }); }); - describe.skip('setEvaluationListener', function () { + describe('setEvaluationListener', function () { it('allows to set evaluation listener for runtime', async function () { const evalListener = { onPrompt: sinon.spy(() => 'password123'), diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index d93f38d5b..f05e1996d 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -13,6 +13,7 @@ import path from 'path'; import { EventEmitter, once } from 'events'; import type { Caller } from './rpc'; import { createCaller, cancel } from './rpc'; +import { ChildProcessEvaluationListener } from './child-process-evaluation-listener'; import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './child-process'; import { deserializeEvaluationResult, @@ -75,6 +76,8 @@ class WorkerRuntime implements Runtime { private childProcessMongoshBus!: ChildProcessMongoshBus; + private childProcessEvaluationListener!: ChildProcessEvaluationListener; + private childProcess!: ChildProcess; private childProcessRuntime!: ChildProcessRuntime; @@ -147,8 +150,8 @@ class WorkerRuntime implements Runtime { // We expect the amount of listeners to be more than the default value of 10 // but probably not more than ~25 (all exposed methods on - // ChildProcessMongoshBus + any concurrent in-flight calls - // on ChildProcessRuntime) at once + // ChildProcessEvaluationListener and ChildProcessMongoshBus + any + // concurrent in-flight calls on ChildProcessRuntime) at once this.childProcess.setMaxListeners(25); this.childProcessRuntime = createCaller( @@ -163,6 +166,11 @@ class WorkerRuntime implements Runtime { this.childProcess ); + this.childProcessEvaluationListener = new ChildProcessEvaluationListener( + this, + this.childProcess + ); + this.childProcessMongoshBus = new ChildProcessMongoshBus( this.eventEmitter, this.childProcess @@ -212,6 +220,10 @@ class WorkerRuntime implements Runtime { this.childProcessRuntime[cancel](); } + if (this.childProcessEvaluationListener) { + this.childProcessEvaluationListener.terminate(); + } + if (this.childProcessMongoshBus) { this.childProcessMongoshBus.terminate(); } From e63f93027fa5dcbb9b845ec3e5a4b10c20251e2b Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 15:32:23 +0200 Subject: [PATCH 04/26] fix emitter tests --- .../src/child-process.ts | 16 +++++++++++++--- .../node-runtime-worker-thread/src/index.spec.ts | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/child-process.ts b/packages/node-runtime-worker-thread/src/child-process.ts index d63460b62..2f24d212e 100644 --- a/packages/node-runtime-worker-thread/src/child-process.ts +++ b/packages/node-runtime-worker-thread/src/child-process.ts @@ -7,11 +7,12 @@ import type { import { ElectronRuntime } from '@mongosh/browser-runtime-electron'; import type { ServiceProvider } from '@mongosh/service-provider-core'; import { CompassServiceProvider } from '@mongosh/service-provider-server'; -import { createCaller, exposeAll } from './rpc'; +import { exposeAll, createCaller } from './rpc'; import { serializeEvaluationResult, deserializeConnectOptions, } from './serializer'; +import type { MongoshBus } from '@mongosh/types'; import type { UNLOCKED } from './lock'; import { Lock } from './lock'; @@ -61,6 +62,15 @@ const evaluationListener = createCaller( } ); +const messageBus: MongoshBus = Object.assign(createCaller(['emit'], process), { + on() { + throw new Error("Can't call `on` method on worker runtime MongoshBus"); + }, + once() { + throw new Error("Can't call `once` method on worker runtime MongoshBus"); + }, +}); + export type WorkerRuntime = Runtime & { init( uri: string, @@ -113,9 +123,9 @@ const workerRuntime: WorkerRuntime = { uri, deserializeConnectOptions(driverOptions), cliOptions, - process + messageBus ); - runtime = new ElectronRuntime(provider as ServiceProvider, process); + runtime = new ElectronRuntime(provider as ServiceProvider, messageBus); runtime.setEvaluationListener(evaluationListener); }, diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index bcd9e48cc..52e834bee 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -211,7 +211,7 @@ describe('WorkerRuntime', function () { }); }); - describe.skip('eventEmitter', function () { + describe('eventEmitter', function () { const testServer = startSharedTestServer(); it('should propagate emitted events from worker', async function () { From 803714876c34a25023eeb5fa7f1b7d97d7ac711b Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 15:35:04 +0200 Subject: [PATCH 05/26] evaluationListener tests --- .../src/child-process.spec.ts | 160 ++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/packages/node-runtime-worker-thread/src/child-process.spec.ts b/packages/node-runtime-worker-thread/src/child-process.spec.ts index 74823c454..c550834c5 100644 --- a/packages/node-runtime-worker-thread/src/child-process.spec.ts +++ b/packages/node-runtime-worker-thread/src/child-process.spec.ts @@ -11,6 +11,7 @@ import type { WorkerRuntime } from './child-process'; import type { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; import { dummyOptions } from './index.spec'; import { type ChildProcess, spawn } from 'child_process'; +import sinon from 'sinon'; chai.use(sinonChai); @@ -474,4 +475,163 @@ describe('child-process', function () { }); }); }); + + describe('evaluationListener', function () { + const spySandbox = sinon.createSandbox(); + + const createSpiedEvaluationListener = () => { + const evalListener = { + onPrint() {}, + onPrompt() { + return '123'; + }, + getConfig() {}, + setConfig() {}, + resetConfig() {}, + validateConfig() {}, + listConfigOptions() { + return ['displayBatchSize']; + }, + onRunInterruptible() {}, + }; + + spySandbox.spy(evalListener, 'onPrint'); + spySandbox.spy(evalListener, 'onPrompt'); + spySandbox.spy(evalListener, 'getConfig'); + spySandbox.spy(evalListener, 'setConfig'); + spySandbox.spy(evalListener, 'resetConfig'); + spySandbox.spy(evalListener, 'validateConfig'); + spySandbox.spy(evalListener, 'listConfigOptions'); + spySandbox.spy(evalListener, 'onRunInterruptible'); + + return evalListener; + }; + + let exposed: Exposed; + + afterEach(function () { + if (exposed) { + exposed[close](); + exposed = null; + } + + spySandbox.restore(); + }); + + describe('onPrint', function () { + it('should be called when shell evaluates `print`', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, childProcess); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + await evaluate('print("Hi!")'); + + expect(evalListener.onPrint).to.have.been.calledWith([ + { printable: 'Hi!', source: undefined, type: null }, + ]); + }); + + it('should correctly serialize bson objects', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, childProcess); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + await evaluate('print(new ObjectId("62a209b0c7dc31e23ab9da45"))'); + + expect(evalListener.onPrint).to.have.been.calledWith([ + { + printable: "ObjectId('62a209b0c7dc31e23ab9da45')", + source: undefined, + type: 'InspectResult', + }, + ]); + }); + }); + + describe('onPrompt', function () { + it('should be called when shell evaluates `passwordPrompt`', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, childProcess); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + const password = await evaluate('passwordPrompt()'); + + expect(evalListener.onPrompt).to.have.been.called; + expect(password.printable).to.equal('123'); + }); + }); + + describe('getConfig', function () { + it('should be called when shell evaluates `config.get()`', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, childProcess); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + + await evaluate('config.get("key")'); + expect(evalListener.getConfig).to.have.been.calledWith('key'); + }); + }); + + describe('setConfig', function () { + it('should be called when shell evaluates `config.set()`', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, childProcess); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + + await evaluate('config.set("displayBatchSize", 200)'); + expect(evalListener.validateConfig).to.have.been.calledWith( + 'displayBatchSize', + 200 + ); + expect(evalListener.setConfig).to.have.been.calledWith( + 'displayBatchSize', + 200 + ); + }); + }); + + describe('resetConfig', function () { + it('should be called when shell evaluates `config.reset()`', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, childProcess); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + + await evaluate('config.reset("displayBatchSize")'); + expect(evalListener.resetConfig).to.have.been.calledWith( + 'displayBatchSize' + ); + }); + }); + + describe('listConfigOptions', function () { + it('should be called when shell evaluates `config[asPrintable]`', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, childProcess); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + + await evaluate(` + var JSSymbol = Object.getOwnPropertySymbols(Array.prototype)[0].constructor; + config[JSSymbol.for("@@mongosh.asPrintable")]()`); + expect(evalListener.listConfigOptions).to.have.been.calledWith(); + }); + }); + }); }); From 19dc9415d705bb2912d6e5dca51f5b3aa8dca480 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 15:35:47 +0200 Subject: [PATCH 06/26] remove interruptor --- package-lock.json | 22 ------------------- .../node-runtime-worker-thread/package.json | 1 - 2 files changed, 23 deletions(-) diff --git a/package-lock.json b/package-lock.json index bf035f8d9..aaddedac3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18164,18 +18164,6 @@ "node": ">= 0.10" } }, - "node_modules/interruptor": { - "version": "1.0.1", - "resolved": "https://registry.npmjs.org/interruptor/-/interruptor-1.0.1.tgz", - "integrity": "sha512-/dScvVlMrCyKwHvYHpgAj5OBFDK79LmqdXcPwcWCnk80pR/ldP4JFD4vU7HdvdtLeR6kHGR+efPwuzjuTlj/zg==", - "hasInstallScript": true, - "dependencies": { - "bindings": "^1.5.0" - }, - "engines": { - "node": ">= 12.0.0" - } - }, "node_modules/into-stream": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/into-stream/-/into-stream-3.1.0.tgz", @@ -31628,7 +31616,6 @@ "version": "0.0.0-dev.0", "license": "Apache-2.0", "dependencies": { - "interruptor": "^1.0.1", "system-ca": "^1.0.3" }, "devDependencies": { @@ -37941,7 +37928,6 @@ "bson": "^6.7.0", "depcheck": "^1.4.3", "eslint": "^7.25.0", - "interruptor": "^1.0.1", "mocha": "^10.2.0", "postmsg-rpc": "^2.4.0", "prettier": "^2.8.8", @@ -46923,14 +46909,6 @@ "integrity": "sha512-Ju0Bz/cEia55xDwUWEa8+olFpCiQoypjnQySseKtmjNrnps3P+xfpUmGr90T7yjlVJmOtybRvPXhKMbHr+fWnw==", "dev": true }, - "interruptor": { - "version": "1.0.1", - "resolved": "https://registry.npmjs.org/interruptor/-/interruptor-1.0.1.tgz", - "integrity": "sha512-/dScvVlMrCyKwHvYHpgAj5OBFDK79LmqdXcPwcWCnk80pR/ldP4JFD4vU7HdvdtLeR6kHGR+efPwuzjuTlj/zg==", - "requires": { - "bindings": "^1.5.0" - } - }, "into-stream": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/into-stream/-/into-stream-3.1.0.tgz", diff --git a/packages/node-runtime-worker-thread/package.json b/packages/node-runtime-worker-thread/package.json index 3702da0a8..67bbf1105 100644 --- a/packages/node-runtime-worker-thread/package.json +++ b/packages/node-runtime-worker-thread/package.json @@ -51,7 +51,6 @@ "webpack-merge": "^5.8.0" }, "dependencies": { - "interruptor": "^1.0.1", "system-ca": "^1.0.3" } } From 8c5bbcd8afea30baf9bc7346166d84500d768cc8 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 15:42:27 +0200 Subject: [PATCH 07/26] clean up --- packages/node-runtime-worker-thread/src/child-process.spec.ts | 2 +- packages/node-runtime-worker-thread/src/child-process.ts | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/node-runtime-worker-thread/src/child-process.spec.ts b/packages/node-runtime-worker-thread/src/child-process.spec.ts index c550834c5..bb22bf98e 100644 --- a/packages/node-runtime-worker-thread/src/child-process.spec.ts +++ b/packages/node-runtime-worker-thread/src/child-process.spec.ts @@ -2,6 +2,7 @@ import path from 'path'; import { once } from 'events'; import chai, { expect } from 'chai'; import sinonChai from 'sinon-chai'; +import sinon from 'sinon'; import { EJSON, ObjectId } from 'bson'; import { startSharedTestServer } from '../../../testing/integration-testing-hooks'; import type { Caller, Exposed } from './rpc'; @@ -11,7 +12,6 @@ import type { WorkerRuntime } from './child-process'; import type { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; import { dummyOptions } from './index.spec'; import { type ChildProcess, spawn } from 'child_process'; -import sinon from 'sinon'; chai.use(sinonChai); diff --git a/packages/node-runtime-worker-thread/src/child-process.ts b/packages/node-runtime-worker-thread/src/child-process.ts index 2f24d212e..47b1bd67a 100644 --- a/packages/node-runtime-worker-thread/src/child-process.ts +++ b/packages/node-runtime-worker-thread/src/child-process.ts @@ -1,3 +1,5 @@ +/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */ + import type { Completion, Runtime, From 8b83717ff1203925ea62f4af7ac345abe18badce Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 15:43:19 +0200 Subject: [PATCH 08/26] istanbul --- packages/node-runtime-worker-thread/src/child-process.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node-runtime-worker-thread/src/child-process.ts b/packages/node-runtime-worker-thread/src/child-process.ts index 47b1bd67a..2b5080b67 100644 --- a/packages/node-runtime-worker-thread/src/child-process.ts +++ b/packages/node-runtime-worker-thread/src/child-process.ts @@ -1,3 +1,4 @@ +/* istanbul ignore file */ /* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */ import type { From f889ac3427b8b4c56e64dec50fd3afa9b057ee76 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 2 Jul 2024 17:18:33 +0200 Subject: [PATCH 09/26] fix ts error --- packages/node-runtime-worker-thread/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index f05e1996d..812648154 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -231,7 +231,7 @@ class WorkerRuntime implements Runtime { async interrupt() { await this.initWorkerPromise; - this.childProcess.kill('SIGINT'); + return this.childProcess.kill('SIGINT'); } async waitForRuntimeToBeReady() { From e470e6900ff9515cfe6f54e338465adcb197548f Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Wed, 3 Jul 2024 09:39:32 +0200 Subject: [PATCH 10/26] fix ci test --- packages/node-runtime-worker-thread/webpack.config.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/node-runtime-worker-thread/webpack.config.js b/packages/node-runtime-worker-thread/webpack.config.js index 7bc44fe66..33b416629 100644 --- a/packages/node-runtime-worker-thread/webpack.config.js +++ b/packages/node-runtime-worker-thread/webpack.config.js @@ -16,15 +16,12 @@ const config = { 'mongodb-client-encryption': 'commonjs2 mongodb-client-encryption', kerberos: 'commonjs2 kerberos', snappy: 'commonjs2 snappy', - interruptor: 'commonjs2 interruptor', 'os-dns-native': 'commonjs2 os-dns-native', 'system-ca': 'commonjs2 system-ca', }, }; -module.exports = ['index', 'child-process-proxy', 'worker-runtime'].map( - (entry) => ({ - entry: { [entry]: path.resolve(__dirname, 'src', `${entry}.ts`) }, - ...merge(baseWebpackConfig, config), - }) -); +module.exports = ['index', 'child-process'].map((entry) => ({ + entry: { [entry]: path.resolve(__dirname, 'src', `${entry}.ts`) }, + ...merge(baseWebpackConfig, config), +})); From 7cf0c0b3866a526c6653e5a6e748a25a822361d2 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 22 Jul 2024 19:18:18 +0200 Subject: [PATCH 11/26] context tests --- .../src/index.spec.ts | 203 ++++++++++++------ 1 file changed, 142 insertions(+), 61 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index 52e834bee..a4e17204d 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -291,85 +291,166 @@ describe('WorkerRuntime', function () { }); describe('interrupt', function () { - it('should interrupt in-flight async tasks', async function () { - runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { - nodb: true, + context('async tasks', function () { + it('should interrupt in-flight tasks', async function () { + runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { + nodb: true, + }); + + await runtime.waitForRuntimeToBeReady(); + + let err: Error; + + try { + await Promise.all([ + runtime.evaluate('sleep(1000000)'), + (async () => { + // This is flaky when not enought time given to the worker to + // finish the sync part of the work. If it causes too much issues + // it would be okay to disable this test completely + await sleep(5000); + await runtime.interrupt(); + })(), + ]); + } catch (e: any) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err) + .to.have.property('message') + .match(/Async script execution was interrupted/); }); - await runtime.waitForRuntimeToBeReady(); + it('should allow to evaluate again after interruption', async function () { + runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { + nodb: true, + }); - let err: Error; + await runtime.waitForRuntimeToBeReady(); - try { - await Promise.all([ - runtime.evaluate('sleep(1000000)'), - (async () => { - // This is flaky when not enought time given to the worker to - // finish the sync part of the work. If it causes too much issues - // it would be okay to disable this test completely - await sleep(5000); - await runtime.interrupt(); - })(), - ]); - } catch (e: any) { - err = e; - } + try { + await Promise.all([ + runtime.evaluate('sleep(1000000)'), + (async () => { + await sleep(200); + await runtime.interrupt(); + })(), + ]); + } catch (e: any) { + // ignore + } - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/Async script execution was interrupted/); - }); + const result = await runtime.evaluate('1+1'); - it('should interrupt in-flight synchronous tasks', async function () { - runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { - nodb: true, + expect(result).to.have.property('printable', 2); }); - await runtime.waitForRuntimeToBeReady(); + it('should preserve the context after interruption', async function () { + runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { + nodb: true, + }); - let err: Error; + await runtime.waitForRuntimeToBeReady(); - try { - await Promise.all([ - runtime.evaluate('while(true){}'), - (async () => { - await sleep(200); - await runtime.interrupt(); - })(), - ]); - } catch (e: any) { - err = e; - } + await runtime.evaluate('let x = 1'); + await runtime.evaluate('x = x + 2'); - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/Script execution was interrupted/); + try { + await Promise.all([ + runtime.evaluate('sleep(1000000)'), + (async () => { + await sleep(200); + await runtime.interrupt(); + })(), + ]); + } catch (e: any) { + // ignore + } + + const result = await runtime.evaluate('x + 3'); + + expect(result).to.have.property('printable', 6); + }); }); - it('should allow to evaluate again after interruption', async function () { - runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { - nodb: true, + context('sync tasks', function () { + it('should interrupt in-flight tasks', async function () { + runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { + nodb: true, + }); + + await runtime.waitForRuntimeToBeReady(); + + let err: Error; + + try { + await Promise.all([ + runtime.evaluate('while(true){}'), + (async () => { + await sleep(200); + await runtime.interrupt(); + })(), + ]); + } catch (e: any) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err) + .to.have.property('message') + .match(/Script execution was interrupted/); + }); + + it('should allow to evaluate again after interruption', async function () { + runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { + nodb: true, + }); + + await runtime.waitForRuntimeToBeReady(); + + try { + await Promise.all([ + runtime.evaluate('while(true){}'), + (async () => { + await sleep(200); + await runtime.interrupt(); + })(), + ]); + } catch (e: any) { + // ignore + } + + const result = await runtime.evaluate('1+1'); + + expect(result).to.have.property('printable', 2); }); - await runtime.waitForRuntimeToBeReady(); + it('should preserve the context after interruption', async function () { + runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { + nodb: true, + }); - try { - await Promise.all([ - runtime.evaluate('while(true){}'), - (async () => { - await sleep(200); - await runtime.interrupt(); - })(), - ]); - } catch (e: any) { - // ignore - } + await runtime.waitForRuntimeToBeReady(); - const result = await runtime.evaluate('1+1'); + await runtime.evaluate('let x = 1'); + await runtime.evaluate('x = x + 2'); + + try { + await Promise.all([ + runtime.evaluate('while(true){}'), + (async () => { + await sleep(200); + await runtime.interrupt(); + })(), + ]); + } catch (e: any) { + // ignore + } - expect(result).to.have.property('printable', 2); + const result = await runtime.evaluate('x + 3'); + expect(result).to.have.property('printable', 6); + }); }); }); }); From 8bfbe5dcd5c29e0077a7febbfc3cdaca5d5f436c Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Fri, 26 Jul 2024 10:01:02 -0400 Subject: [PATCH 12/26] chore(node-runtime-worker-thread): remove child process indirection, use worker_threads directly --- package-lock.json | 22 ++ package.json | 4 +- .../node-runtime-worker-thread/package.json | 1 + .../src/index.spec.ts | 51 +++- .../node-runtime-worker-thread/src/index.ts | 225 ++++++++---------- ...h-bus.ts => worker-process-mongosh-bus.ts} | 12 +- ...process.spec.ts => worker-runtime.spec.ts} | 133 +++++++++-- .../{child-process.ts => worker-runtime.ts} | 132 ++++++---- ...s => worker-thread-evaluation-listener.ts} | 8 +- .../webpack.config.js | 2 +- 10 files changed, 366 insertions(+), 224 deletions(-) rename packages/node-runtime-worker-thread/src/{child-process-mongosh-bus.ts => worker-process-mongosh-bus.ts} (59%) rename packages/node-runtime-worker-thread/src/{child-process.spec.ts => worker-runtime.spec.ts} (83%) rename packages/node-runtime-worker-thread/src/{child-process.ts => worker-runtime.ts} (57%) rename packages/node-runtime-worker-thread/src/{child-process-evaluation-listener.ts => worker-thread-evaluation-listener.ts} (90%) diff --git a/package-lock.json b/package-lock.json index aaddedac3..c3ffea2c8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18164,6 +18164,18 @@ "node": ">= 0.10" } }, + "node_modules/interruptor": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/interruptor/-/interruptor-1.0.2.tgz", + "integrity": "sha512-sn7EmHLEsE0sFn/0xShWiyd7SUDQaZV9MIWpdm2jnnC1vGRAmZeywv2m+AOtZde6zUTZFLaQnlf1SCIGu7kQWA==", + "hasInstallScript": true, + "dependencies": { + "bindings": "^1.5.0" + }, + "engines": { + "node": ">= 12.0.0" + } + }, "node_modules/into-stream": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/into-stream/-/into-stream-3.1.0.tgz", @@ -31616,6 +31628,7 @@ "version": "0.0.0-dev.0", "license": "Apache-2.0", "dependencies": { + "interruptor": "^1.0.1", "system-ca": "^1.0.3" }, "devDependencies": { @@ -37928,6 +37941,7 @@ "bson": "^6.7.0", "depcheck": "^1.4.3", "eslint": "^7.25.0", + "interruptor": "^1.0.1", "mocha": "^10.2.0", "postmsg-rpc": "^2.4.0", "prettier": "^2.8.8", @@ -46909,6 +46923,14 @@ "integrity": "sha512-Ju0Bz/cEia55xDwUWEa8+olFpCiQoypjnQySseKtmjNrnps3P+xfpUmGr90T7yjlVJmOtybRvPXhKMbHr+fWnw==", "dev": true }, + "interruptor": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/interruptor/-/interruptor-1.0.2.tgz", + "integrity": "sha512-sn7EmHLEsE0sFn/0xShWiyd7SUDQaZV9MIWpdm2jnnC1vGRAmZeywv2m+AOtZde6zUTZFLaQnlf1SCIGu7kQWA==", + "requires": { + "bindings": "^1.5.0" + } + }, "into-stream": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/into-stream/-/into-stream-3.1.0.tgz", diff --git a/package.json b/package.json index 6d28a2471..e1d15a69c 100644 --- a/package.json +++ b/package.json @@ -136,14 +136,14 @@ "workspaces": [ "configs/eslint-config-mongosh", "configs/tsconfig-mongosh", + "packages/java-shell", + "packages/js-multiline-to-singleline", "scripts/docker", "packages/async-rewriter2", "packages/build", "packages/errors", "packages/history", "packages/import-node-fetch", - "packages/java-shell", - "packages/js-multiline-to-singleline", "packages/types", "packages/i18n", "packages/logging", diff --git a/packages/node-runtime-worker-thread/package.json b/packages/node-runtime-worker-thread/package.json index 67bbf1105..3702da0a8 100644 --- a/packages/node-runtime-worker-thread/package.json +++ b/packages/node-runtime-worker-thread/package.json @@ -51,6 +51,7 @@ "webpack-merge": "^5.8.0" }, "dependencies": { + "interruptor": "^1.0.1", "system-ca": "^1.0.3" } } diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index a4e17204d..f601f908f 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -28,7 +28,17 @@ describe('WorkerRuntime', function () { afterEach(async function () { if (runtime) { - await runtime.terminate(); + // There is a Node.js bug that causes worker process to still be ref-ed + // after termination. To work around that, we are unrefing worker manually + // *immediately* after terminate method is called even though it should + // not be necessary. If this is not done in rare cases our test suite can + // get stuck. Even though the issue is fixed we would still need to keep + // this workaround for compat reasons. + // + // See: https://github.com/nodejs/node/pull/37319 + const terminationPromise = runtime.terminate(); + runtime['workerProcess'].unref(); + await terminationPromise; runtime = null; } }); @@ -43,11 +53,11 @@ describe('WorkerRuntime', function () { afterEach(function () { delete process - .env.CHILD_PROCESS_PROXY_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING; + .env.WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING; }); - it('should return init error if child process failed to spawn', async function () { - process.env.CHILD_PROCESS_PROXY_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING = + it('should return init error if worker thread failed to spawn', async function () { + process.env.WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING = brokenScript; runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { @@ -65,7 +75,7 @@ describe('WorkerRuntime', function () { expect(err).to.be.instanceof(Error); expect(err) .to.have.property('message') - .match(/Child process failed to start/); + .match(/Worker thread failed to start with/); }); }); @@ -251,21 +261,40 @@ describe('WorkerRuntime', function () { // We will be testing a bunch of private props that can be accessed only with // strings to make TS happy - it('should terminate child process', async function () { + it('should terminate the worker thread', async function () { const runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { nodb: true, }); + await runtime.waitForRuntimeToBeReady(); + expect(isRunning(runtime['workerProcess'].threadId)).to.not.equal(-1); + expect(isRunning(runtime['workerProcess'].threadId)).to.not.equal( + undefined + ); + + let resolvePromise; + const exitCalledPromise = new Promise((resolve) => { + resolvePromise = resolve; + }); + runtime['workerProcess'].on('exit', () => { + resolvePromise(); + }); + + // await waitFor(() => runtime['workerProcess'] !== undefined); await runtime.terminate(); - expect(runtime['childProcess']).to.have.property('killed', true); - expect(isRunning(runtime['childProcess'].pid)).to.equal(false); + + await exitCalledPromise; + + // expect(runtime['workerProcess']).to.have.property('killed', true); + // expect(isRunning(runtime['workerProcess'].threadId)).to.equal(false); + expect(runtime['workerProcess'].threadId).to.equal(-1); }); - it('should remove all listeners from childProcess', async function () { + it('should remove all listeners from workerProcess', async function () { const runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { nodb: true, }); await runtime.terminate(); - expect(runtime['childProcess'].listenerCount('message')).to.equal(0); + expect(runtime['workerProcess'].listenerCount('message')).to.equal(0); }); it('should cancel any in-flight runtime calls', async function () { @@ -305,7 +334,7 @@ describe('WorkerRuntime', function () { await Promise.all([ runtime.evaluate('sleep(1000000)'), (async () => { - // This is flaky when not enought time given to the worker to + // This is flaky when not enough time is given to the worker to // finish the sync part of the work. If it causes too much issues // it would be okay to disable this test completely await sleep(5000); diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index 812648154..997110dd7 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -1,8 +1,6 @@ /* istanbul ignore file */ -/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */ +/* ^^^ we test the dist directly, so istanbul can't calculate the coverage correctly */ -import type { ChildProcess, SpawnOptionsWithoutStdio } from 'child_process'; -import { spawn } from 'child_process'; import type { Runtime, RuntimeEvaluationListener, @@ -12,192 +10,161 @@ import type { MongoshBus } from '@mongosh/types'; import path from 'path'; import { EventEmitter, once } from 'events'; import type { Caller } from './rpc'; -import { createCaller, cancel } from './rpc'; -import { ChildProcessEvaluationListener } from './child-process-evaluation-listener'; -import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './child-process'; +import { createCaller, cancel, exposeAll } from './rpc'; +import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './worker-runtime'; import { deserializeEvaluationResult, serializeConnectOptions, } from './serializer'; -import { ChildProcessMongoshBus } from './child-process-mongosh-bus'; import type { CompassServiceProvider } from '@mongosh/service-provider-server'; +import { SHARE_ENV, Worker } from 'worker_threads'; +import type { WorkerOptions } from 'worker_threads'; +import type { InterruptHandle } from 'interruptor'; +import { interrupt as nativeInterrupt } from 'interruptor'; +import { WorkerThreadEvaluationListener } from './worker-thread-evaluation-listener'; +import { WorkerProcessMongoshBus } from './worker-process-mongosh-bus'; type DevtoolsConnectOptions = Parameters< (typeof CompassServiceProvider)['connect'] >[1]; -type ChildProcessRuntime = Caller; - -async function kill( - childProcess: ChildProcess, - code: NodeJS.Signals | number = 'SIGTERM' -) { - childProcess.kill(code); - if (childProcess.exitCode === null && childProcess.signalCode === null) { - await once(childProcess, 'exit'); - } -} - -function parseStderrToError(str: string): Error | null { - const [, errorMessageWithStack] = str - .split(/^\s*\^\s*$/m) - .map((part) => part.trim()); +type WorkerThreadRuntime = Caller; - if (errorMessageWithStack) { - const e = new Error(); - const errorHeader = - errorMessageWithStack.substring( - 0, - errorMessageWithStack.search(/^\s*at/m) - ) || errorMessageWithStack; +const workerRuntimeSrcPath = path.resolve(__dirname, 'worker-runtime.js'); - const [name, ...message] = errorHeader.split(': '); - - e.name = name; - e.message = message.join(': ').trim(); - e.stack = errorMessageWithStack; +const workerReadyPromise = async (workerProcess: Worker): Promise => { + const waitForReadyMessage = async () => { + let msg: string; + while (([msg] = await once(workerProcess, 'message'))) { + if (msg === 'ready') return; + } + }; - return e; - } + const waitForError = async () => { + const [err] = await once(workerProcess, 'error'); + if (err) { + err.message = `Worker thread failed to start with the following error: ${err.message}`; + throw err; + } + }; - return null; -} + await Promise.race([waitForReadyMessage(), waitForError()]); +}; class WorkerRuntime implements Runtime { private initOptions: { uri: string; driverOptions: DevtoolsConnectOptions; cliOptions: { nodb?: boolean }; - spawnOptions: SpawnOptionsWithoutStdio; + workerOptions: WorkerOptions; }; evaluationListener: RuntimeEvaluationListener | null = null; private eventEmitter: MongoshBus; - private childProcessMongoshBus!: ChildProcessMongoshBus; - - private childProcessEvaluationListener!: ChildProcessEvaluationListener; - - private childProcess!: ChildProcess; + private workerProcess!: Worker; - private childProcessRuntime!: ChildProcessRuntime; + private workerProcessRuntime!: WorkerThreadRuntime; private initWorkerPromise: Promise; - private childProcessProxySrcPath: string = - process.env - .CHILD_PROCESS_PROXY_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING || - path.resolve(__dirname, 'child-process.js'); + private workerThreadEvaluationListener!: WorkerThreadEvaluationListener; + + private workerProcessMongoshBus!: WorkerProcessMongoshBus; constructor( uri: string, driverOptions: DevtoolsConnectOptions, cliOptions: { nodb?: boolean } = {}, - spawnOptions: SpawnOptionsWithoutStdio = {}, + workerOptions: WorkerOptions = {}, eventEmitter: MongoshBus = new EventEmitter() ) { - this.initOptions = { uri, driverOptions, cliOptions, spawnOptions }; + this.initOptions = { uri, driverOptions, cliOptions, workerOptions }; this.eventEmitter = eventEmitter; this.initWorkerPromise = this.initWorker(); } private async initWorker() { - const { uri, driverOptions, cliOptions, spawnOptions } = this.initOptions; - - this.childProcess = spawn( - process.execPath, - [this.childProcessProxySrcPath], - { - stdio: ['inherit', 'inherit', 'pipe', 'ipc'], - ...spawnOptions, - } + const workerProcess = new Worker( + process.env.WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING || + workerRuntimeSrcPath, + { env: SHARE_ENV } ); + this.workerProcess = workerProcess; - const waitForReadyMessage = async () => { - let msg: string; - while (([msg] = await once(this.childProcess, 'message'))) { - if (msg === 'ready') return; - } - }; - - let spawnError = ''; + await workerReadyPromise(this.workerProcess); - this.childProcess?.stderr?.setEncoding('utf8')?.on('data', (chunk) => { - spawnError += chunk; - }); + workerProcess.setMaxListeners(25); - const waitForError = async () => { - const [exitCode] = await once(this.childProcess, 'exit'); + const { interrupt } = createCaller(['interrupt'], workerProcess); - if (exitCode) { - let error = parseStderrToError(spawnError); + let interruptHandle: InterruptHandle | null = null; - if (error) { - error.message = `Child process failed to start with the following error: ${error.message}`; - } else { - error = new Error( - `Worker runtime failed to start: child process exited with code ${ - exitCode as number | string - }` - ); - } - - throw error; + this.workerProcessRuntime = Object.assign( + createCaller( + [ + 'init', + 'evaluate', + 'getCompletions', + 'getShellPrompt', + 'setEvaluationListener', + 'interrupt', + ], + workerProcess + ), + { + interrupt(): boolean { + if (interruptHandle) { + nativeInterrupt(interruptHandle); + return true; + } + + return interrupt(); + }, } - }; - - await Promise.race([waitForReadyMessage(), waitForError()]); - - // We expect the amount of listeners to be more than the default value of 10 - // but probably not more than ~25 (all exposed methods on - // ChildProcessEvaluationListener and ChildProcessMongoshBus + any - // concurrent in-flight calls on ChildProcessRuntime) at once - this.childProcess.setMaxListeners(25); - - this.childProcessRuntime = createCaller( - [ - 'init', - 'evaluate', - 'getCompletions', - 'setEvaluationListener', - 'getShellPrompt', - 'interrupt', - ], - this.childProcess ); - this.childProcessEvaluationListener = new ChildProcessEvaluationListener( + this.workerThreadEvaluationListener = new WorkerThreadEvaluationListener( this, - this.childProcess + this.workerProcess + ); + + exposeAll( + { + onRunInterruptible(handle: InterruptHandle | null) { + interruptHandle = handle; + }, + }, + workerProcess ); - this.childProcessMongoshBus = new ChildProcessMongoshBus( + this.workerProcessMongoshBus = new WorkerProcessMongoshBus( this.eventEmitter, - this.childProcess + this.workerProcess ); - await this.childProcessRuntime.init( - uri, - serializeConnectOptions(driverOptions), - cliOptions + await this.workerProcessRuntime.init( + this.initOptions.uri, + serializeConnectOptions(this.initOptions.driverOptions), + this.initOptions.cliOptions ); } async evaluate(code: string): Promise { await this.initWorkerPromise; return deserializeEvaluationResult( - await this.childProcessRuntime.evaluate(code) + await this.workerProcessRuntime.evaluate(code) ); } async getCompletions(code: string) { await this.initWorkerPromise; - return await this.childProcessRuntime.getCompletions(code); + return await this.workerProcessRuntime.getCompletions(code); } async getShellPrompt() { await this.initWorkerPromise; - return await this.childProcessRuntime.getShellPrompt(); + return await this.workerProcessRuntime.getShellPrompt(); } setEvaluationListener(listener: RuntimeEvaluationListener | null) { @@ -210,28 +177,30 @@ class WorkerRuntime implements Runtime { try { await this.initWorkerPromise; } catch { - // In case child process encountered an error during init we still want - // to clean up whatever possible + // In case the worker thread encountered an error during init + // we still want to clean up whatever possible. } - await kill(this.childProcess); + if (this.workerProcessRuntime) { + this.workerProcessRuntime[cancel](); + } - if (this.childProcessRuntime) { - this.childProcessRuntime[cancel](); + if (this.workerProcess) { + await this.workerProcess.terminate(); } - if (this.childProcessEvaluationListener) { - this.childProcessEvaluationListener.terminate(); + if (this.workerThreadEvaluationListener) { + this.workerThreadEvaluationListener.terminate(); } - if (this.childProcessMongoshBus) { - this.childProcessMongoshBus.terminate(); + if (this.workerProcessMongoshBus) { + this.workerProcessMongoshBus.terminate(); } } async interrupt() { await this.initWorkerPromise; - return this.childProcess.kill('SIGINT'); + return this.workerProcessRuntime.interrupt(); } async waitForRuntimeToBeReady() { diff --git a/packages/node-runtime-worker-thread/src/child-process-mongosh-bus.ts b/packages/node-runtime-worker-thread/src/worker-process-mongosh-bus.ts similarity index 59% rename from packages/node-runtime-worker-thread/src/child-process-mongosh-bus.ts rename to packages/node-runtime-worker-thread/src/worker-process-mongosh-bus.ts index c09812bec..6b8f60983 100644 --- a/packages/node-runtime-worker-thread/src/child-process-mongosh-bus.ts +++ b/packages/node-runtime-worker-thread/src/worker-process-mongosh-bus.ts @@ -1,25 +1,25 @@ -import type { ChildProcess } from 'child_process'; +import type { Worker } from 'worker_threads'; import type { MongoshBus } from '@mongosh/types'; import type { Exposed } from './rpc'; import { exposeAll, close } from './rpc'; -export class ChildProcessMongoshBus { +export class WorkerProcessMongoshBus { exposedEmitter: Exposed; - constructor(eventEmitter: MongoshBus, childProcess: ChildProcess) { + constructor(eventEmitter: MongoshBus, worker: Worker) { const exposedEmitter: Exposed = exposeAll( { emit(...args) { eventEmitter.emit(...args); }, on() { - throw new Error("Can't use `on` method on ChildProcessMongoshBus"); + throw new Error("Can't use `on` method on WorkerProcessMongoshBus"); }, once() { - throw new Error("Can't use `once` method on ChildProcessMongoshBus"); + throw new Error("Can't use `once` method on WorkerProcessMongoshBus"); }, }, - childProcess + worker ); this.exposedEmitter = exposedEmitter; } diff --git a/packages/node-runtime-worker-thread/src/child-process.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts similarity index 83% rename from packages/node-runtime-worker-thread/src/child-process.spec.ts rename to packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index bb22bf98e..feb286204 100644 --- a/packages/node-runtime-worker-thread/src/child-process.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -1,5 +1,6 @@ import path from 'path'; import { once } from 'events'; +import { Worker } from 'worker_threads'; import chai, { expect } from 'chai'; import sinonChai from 'sinon-chai'; import sinon from 'sinon'; @@ -8,10 +9,10 @@ import { startSharedTestServer } from '../../../testing/integration-testing-hook import type { Caller, Exposed } from './rpc'; import { cancel, close, createCaller, exposeAll } from './rpc'; import { deserializeEvaluationResult } from './serializer'; -import type { WorkerRuntime } from './child-process'; +import type { WorkerRuntime } from './worker-runtime'; import type { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; +import { interrupt } from 'interruptor'; import { dummyOptions } from './index.spec'; -import { type ChildProcess, spawn } from 'child_process'; chai.use(sinonChai); @@ -20,18 +21,20 @@ const workerThreadModule = path.resolve( __dirname, '..', 'dist', - 'child-process.js' + 'worker-runtime.js' ); -describe('child-process', function () { - let childProcess: ChildProcess; +function sleep(ms: number) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +describe('worker', function () { + let worker: Worker; let caller: Caller; beforeEach(async function () { - childProcess = spawn(process.execPath, [workerThreadModule], { - stdio: ['inherit', 'inherit', 'pipe', 'ipc'], - }); - await once(childProcess, 'message'); + worker = new Worker(workerThreadModule); + await once(worker, 'message'); caller = createCaller( [ @@ -40,8 +43,9 @@ describe('child-process', function () { 'getCompletions', 'getShellPrompt', 'setEvaluationListener', + 'interrupt', ], - childProcess + worker ); const origEvaluate = caller.evaluate; caller.evaluate = (code: string): Promise & { cancel(): void } => { @@ -51,9 +55,20 @@ describe('child-process', function () { }; }); - afterEach(function () { - if (childProcess) { - childProcess.kill(); + afterEach(async function () { + if (worker) { + // There is a Node.js bug that causes worker process to still be ref-ed + // after termination. To work around that, we are unrefing worker manually + // *immediately* after terminate method is called even though it should + // not be necessary. If this is not done in rare cases our test suite can + // get stuck. Even though the issue is fixed we would still need to keep + // this workaround for compat reasons. + // + // See: https://github.com/nodejs/node/pull/37319 + const terminationPromise = worker.terminate(); + worker.unref(); + await terminationPromise; + worker = null; } if (caller) { @@ -336,7 +351,7 @@ describe('child-process', function () { getConfig() {}, validateConfig() {}, }, - childProcess + worker ); const { init, evaluate } = caller; @@ -523,7 +538,7 @@ describe('child-process', function () { const { init, evaluate } = caller; const evalListener = createSpiedEvaluationListener(); - exposed = exposeAll(evalListener, childProcess); + exposed = exposeAll(evalListener, worker); await init('mongodb://nodb/', dummyOptions, { nodb: true }); await evaluate('print("Hi!")'); @@ -537,7 +552,7 @@ describe('child-process', function () { const { init, evaluate } = caller; const evalListener = createSpiedEvaluationListener(); - exposed = exposeAll(evalListener, childProcess); + exposed = exposeAll(evalListener, worker); await init('mongodb://nodb/', dummyOptions, { nodb: true }); await evaluate('print(new ObjectId("62a209b0c7dc31e23ab9da45"))'); @@ -557,7 +572,7 @@ describe('child-process', function () { const { init, evaluate } = caller; const evalListener = createSpiedEvaluationListener(); - exposed = exposeAll(evalListener, childProcess); + exposed = exposeAll(evalListener, worker); await init('mongodb://nodb/', dummyOptions, { nodb: true }); const password = await evaluate('passwordPrompt()'); @@ -572,7 +587,7 @@ describe('child-process', function () { const { init, evaluate } = caller; const evalListener = createSpiedEvaluationListener(); - exposed = exposeAll(evalListener, childProcess); + exposed = exposeAll(evalListener, worker); await init('mongodb://nodb/', dummyOptions, { nodb: true }); @@ -586,7 +601,7 @@ describe('child-process', function () { const { init, evaluate } = caller; const evalListener = createSpiedEvaluationListener(); - exposed = exposeAll(evalListener, childProcess); + exposed = exposeAll(evalListener, worker); await init('mongodb://nodb/', dummyOptions, { nodb: true }); @@ -607,7 +622,7 @@ describe('child-process', function () { const { init, evaluate } = caller; const evalListener = createSpiedEvaluationListener(); - exposed = exposeAll(evalListener, childProcess); + exposed = exposeAll(evalListener, worker); await init('mongodb://nodb/', dummyOptions, { nodb: true }); @@ -623,7 +638,7 @@ describe('child-process', function () { const { init, evaluate } = caller; const evalListener = createSpiedEvaluationListener(); - exposed = exposeAll(evalListener, childProcess); + exposed = exposeAll(evalListener, worker); await init('mongodb://nodb/', dummyOptions, { nodb: true }); @@ -633,5 +648,81 @@ describe('child-process', function () { expect(evalListener.listConfigOptions).to.have.been.calledWith(); }); }); + + describe('onRunInterruptible', function () { + it('should call callback when interruptible evaluation starts and ends', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, worker); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + await evaluate('1+1'); + + const [firstCall, secondCall] = ( + evalListener.onRunInterruptible as sinon.SinonSpy + ).args; + + expect(firstCall[0]).to.have.property('__id'); + expect(secondCall[0]).to.equal(null); + }); + + it('should return a handle that allows to interrupt the evaluation', async function () { + const { init, evaluate } = caller; + const evalListener = createSpiedEvaluationListener(); + + exposed = exposeAll(evalListener, worker); + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + + let err: Error; + + try { + await Promise.all([ + evaluate('while(true){}'), + (async () => { + await sleep(50); + const handle = (evalListener.onRunInterruptible as sinon.SinonSpy) + .args[0][0]; + interrupt(handle); + })(), + ]); + } catch (e: any) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err) + .to.have.property('message') + .match(/Script execution was interrupted/); + }); + }); + }); + + describe('interrupt', function () { + it('should interrupt in-flight async tasks', async function () { + const { init, evaluate, interrupt } = caller; + + await init('mongodb://nodb/', dummyOptions, { nodb: true }); + + let err: Error; + + try { + await Promise.all([ + evaluate('sleep(100000)'), + (async () => { + await sleep(10); + await interrupt(); + })(), + ]); + } catch (e: any) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err) + .to.have.property('message') + .match(/Async script execution was interrupted/); + }); }); }); diff --git a/packages/node-runtime-worker-thread/src/child-process.ts b/packages/node-runtime-worker-thread/src/worker-runtime.ts similarity index 57% rename from packages/node-runtime-worker-thread/src/child-process.ts rename to packages/node-runtime-worker-thread/src/worker-runtime.ts index 2b5080b67..943cd6dd2 100644 --- a/packages/node-runtime-worker-thread/src/child-process.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.ts @@ -1,6 +1,7 @@ /* istanbul ignore file */ -/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */ +/* ^^^ we test the dist directly, so istanbul can't calculate the coverage correctly */ +import { parentPort, isMainThread } from 'worker_threads'; import type { Completion, Runtime, @@ -18,11 +19,16 @@ import { import type { MongoshBus } from '@mongosh/types'; import type { UNLOCKED } from './lock'; import { Lock } from './lock'; +import type { InterruptHandle } from 'interruptor'; +import { runInterruptible } from 'interruptor'; type DevtoolsConnectOptions = Parameters< (typeof CompassServiceProvider)['connect'] >[1]; -type EvaluationResult = void | RuntimeEvaluationResult | UNLOCKED; + +if (!parentPort || isMainThread) { + throw new Error('Worker runtime can be used only in a worker thread'); +} let runtime: Runtime | null = null; let provider: ServiceProvider | null = null; @@ -39,7 +45,11 @@ function ensureRuntime(methodName: string): Runtime { return runtime; } -const evaluationListener = createCaller( +export type WorkerRuntimeEvaluationListener = RuntimeEvaluationListener & { + onRunInterruptible(handle: InterruptHandle | null): void; +}; + +const evaluationListener = createCaller( [ 'onPrint', 'onPrompt', @@ -50,8 +60,9 @@ const evaluationListener = createCaller( 'listConfigOptions', 'onClearCommand', 'onExit', + 'onRunInterruptible', ], - process, + parentPort, { onPrint: function ( results: RuntimeEvaluationResult[] @@ -65,14 +76,17 @@ const evaluationListener = createCaller( } ); -const messageBus: MongoshBus = Object.assign(createCaller(['emit'], process), { - on() { - throw new Error("Can't call `on` method on worker runtime MongoshBus"); - }, - once() { - throw new Error("Can't call `once` method on worker runtime MongoshBus"); - }, -}); +const messageBus: MongoshBus = Object.assign( + createCaller(['emit'], parentPort), + { + on() { + throw new Error("Can't call `on` method on worker runtime MongoshBus"); + }, + once() { + throw new Error("Can't call `once` method on worker runtime MongoshBus"); + }, + } +); export type WorkerRuntime = Runtime & { init( @@ -80,33 +94,9 @@ export type WorkerRuntime = Runtime & { driverOptions?: DevtoolsConnectOptions, cliOptions?: { nodb?: boolean } ): Promise; -}; - -// If we were interrupted, we can't know which newly require()ed modules -// have successfully been loaded, so we restore everything to its -// previous state. -function clearRequireCache(previousRequireCache: string[]) { - for (const key of Object.keys(require.cache)) { - if (!previousRequireCache.includes(key)) { - delete require.cache[key]; - } - } -} - -function throwIfInterrupted( - result: EvaluationResult, - previousRequireCache: string[] -): asserts result is RuntimeEvaluationResult { - if (evaluationLock.isUnlockToken(result)) { - clearRequireCache(previousRequireCache); - throw new Error('Async script execution was interrupted'); - } - if (typeof result === 'undefined') { - clearRequireCache(previousRequireCache); - throw new Error('Script execution was interrupted'); - } -} + interrupt(): boolean; +}; const workerRuntime: WorkerRuntime = { async init( @@ -139,18 +129,52 @@ const workerRuntime: WorkerRuntime = { ); } + let interrupted = true; + let evaluationPromise: void | Promise; const previousRequireCache = Object.keys(require.cache); - let result: EvaluationResult; try { - result = await Promise.race([ - ensureRuntime('evaluate').evaluate(code), - evaluationLock.lock(), - ]); + evaluationPromise = runInterruptible((handle) => { + try { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + evaluationListener.onRunInterruptible(handle); + return ensureRuntime('evaluate').evaluate(code); + } finally { + interrupted = false; + } + }); + } finally { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + evaluationListener.onRunInterruptible(null); + + if (interrupted) { + // If we were interrupted, we can't know which newly require()ed modules + // have successfully been loaded, so we restore everything to its + // previous state. + for (const key of Object.keys(require.cache)) { + if (!previousRequireCache.includes(key)) { + delete require.cache[key]; + } + } + } + } + + let result: void | RuntimeEvaluationResult | UNLOCKED; + + try { + result = await Promise.race([evaluationPromise, evaluationLock.lock()]); } finally { evaluationLock.unlock(); } - throwIfInterrupted(result, previousRequireCache); + + if (evaluationLock.isUnlockToken(result)) { + throw new Error('Async script execution was interrupted'); + } + + if (typeof result === 'undefined' || interrupted === true) { + throw new Error('Script execution was interrupted'); + } + return serializeEvaluationResult(result); }, @@ -167,16 +191,22 @@ const workerRuntime: WorkerRuntime = { 'Evaluation listener can not be directly set on the worker runtime' ); }, + + interrupt() { + return evaluationLock.unlock(); + }, }; -exposeAll(workerRuntime, process); +// We expect the amount of listeners to be more than the default value of 10 but +// probably not more than ~25 (all exposed methods on +// WorkerThreadEvaluationListener and WorkerThreadMongoshBus + any concurrent +// in-flight calls on WorkerThreadRuntime) at once +parentPort.setMaxListeners(25); -// For async tasks, then an interrupt is received, we need to unlock the -// evaluation lock so that it resolves and workerRuntime.evaluate throws. -process.on('SIGINT', () => { - evaluationLock.unlock(); -}); +exposeAll(workerRuntime, parentPort); process.nextTick(() => { - process.send?.('ready'); + if (parentPort) { + parentPort.postMessage('ready'); + } }); diff --git a/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts b/packages/node-runtime-worker-thread/src/worker-thread-evaluation-listener.ts similarity index 90% rename from packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts rename to packages/node-runtime-worker-thread/src/worker-thread-evaluation-listener.ts index 6ed6a7441..2cc586f1c 100644 --- a/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts +++ b/packages/node-runtime-worker-thread/src/worker-thread-evaluation-listener.ts @@ -1,18 +1,18 @@ -import type { ChildProcess } from 'child_process'; +import type { Worker } from 'worker_threads'; import type { Exposed } from './rpc'; import { exposeAll, close } from './rpc'; import type { WorkerRuntime } from './index'; import { deserializeEvaluationResult } from './serializer'; import type { RuntimeEvaluationListener } from '@mongosh/browser-runtime-core'; -export class ChildProcessEvaluationListener { +export class WorkerThreadEvaluationListener { exposedListener: Exposed< Required< Omit > >; - constructor(workerRuntime: WorkerRuntime, childProcess: ChildProcess) { + constructor(workerRuntime: WorkerRuntime, worker: Worker) { this.exposedListener = exposeAll( { onPrompt(question, type) { @@ -58,7 +58,7 @@ export class ChildProcessEvaluationListener { ); }, }, - childProcess + worker ); } diff --git a/packages/node-runtime-worker-thread/webpack.config.js b/packages/node-runtime-worker-thread/webpack.config.js index 33b416629..26046c6c1 100644 --- a/packages/node-runtime-worker-thread/webpack.config.js +++ b/packages/node-runtime-worker-thread/webpack.config.js @@ -21,7 +21,7 @@ const config = { }, }; -module.exports = ['index', 'child-process'].map((entry) => ({ +module.exports = ['index'].map((entry) => ({ entry: { [entry]: path.resolve(__dirname, 'src', `${entry}.ts`) }, ...merge(baseWebpackConfig, config), })); From b6932d29245f52da926d070608d0d2706618ae4b Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Fri, 26 Jul 2024 10:11:21 -0400 Subject: [PATCH 13/26] fixup --- package.json | 4 ++-- packages/node-runtime-worker-thread/src/index.spec.ts | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index e1d15a69c..6d28a2471 100644 --- a/package.json +++ b/package.json @@ -136,14 +136,14 @@ "workspaces": [ "configs/eslint-config-mongosh", "configs/tsconfig-mongosh", - "packages/java-shell", - "packages/js-multiline-to-singleline", "scripts/docker", "packages/async-rewriter2", "packages/build", "packages/errors", "packages/history", "packages/import-node-fetch", + "packages/java-shell", + "packages/js-multiline-to-singleline", "packages/types", "packages/i18n", "packages/logging", diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index f601f908f..451859e23 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -279,13 +279,9 @@ describe('WorkerRuntime', function () { resolvePromise(); }); - // await waitFor(() => runtime['workerProcess'] !== undefined); await runtime.terminate(); await exitCalledPromise; - - // expect(runtime['workerProcess']).to.have.property('killed', true); - // expect(isRunning(runtime['workerProcess'].threadId)).to.equal(false); expect(runtime['workerProcess'].threadId).to.equal(-1); }); From 29c024e5b340165690531b343f3c5132e4afadeb Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 02:06:45 +0200 Subject: [PATCH 14/26] wip tests --- package-lock.json | 14 ++- .../node-runtime-worker-thread/package.json | 3 +- .../src/index.spec.ts | 68 ++--------- .../node-runtime-worker-thread/src/index.ts | 67 +++++------ .../src/rpc.spec.ts | 79 +++---------- .../node-runtime-worker-thread/src/rpc.ts | 108 ++---------------- .../src/worker-process-mongosh-bus.ts | 1 - .../src/worker-runtime.spec.ts | 27 ++--- .../src/worker-runtime.ts | 36 +++--- .../src/worker-thread-evaluation-listener.ts | 1 - .../node-runtime-worker-thread/tsconfig.json | 3 +- 11 files changed, 115 insertions(+), 292 deletions(-) diff --git a/package-lock.json b/package-lock.json index c8b90a76f..c378beda9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29919,6 +29919,11 @@ "node": ">= 8" } }, + "node_modules/web-worker": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/web-worker/-/web-worker-1.3.0.tgz", + "integrity": "sha512-BSR9wyRsy/KOValMgd5kMyr3JzpdeoR9KVId8u5GVlTTAtNChlsE4yTxeY7zMdNSyOmoKBv8NH2qeRY9Tg+IaA==" + }, "node_modules/webidl-conversions": { "version": "7.0.0", "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-7.0.0.tgz", @@ -31788,7 +31793,8 @@ "license": "Apache-2.0", "dependencies": { "interruptor": "^1.0.1", - "system-ca": "^2.0.1" + "system-ca": "^2.0.1", + "web-worker": "^1.3.0" }, "devDependencies": { "@mongodb-js/eslint-config-mongosh": "^1.0.0", @@ -38170,6 +38176,7 @@ "postmsg-rpc": "^2.4.0", "prettier": "^2.8.8", "system-ca": "^2.0.1", + "web-worker": "^1.3.0", "webpack-merge": "^5.8.0" } }, @@ -55979,6 +55986,11 @@ "resolved": "https://registry.npmjs.org/web-streams-polyfill/-/web-streams-polyfill-3.3.3.tgz", "integrity": "sha512-d2JWLCivmZYTSIoge9MsgFCZrt571BikcWGYkjC1khllbTeDlGqZ2D8vD8E/lJa8WGWbb7Plm8/XJYV7IJHZZw==" }, + "web-worker": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/web-worker/-/web-worker-1.3.0.tgz", + "integrity": "sha512-BSR9wyRsy/KOValMgd5kMyr3JzpdeoR9KVId8u5GVlTTAtNChlsE4yTxeY7zMdNSyOmoKBv8NH2qeRY9Tg+IaA==" + }, "webidl-conversions": { "version": "7.0.0", "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-7.0.0.tgz", diff --git a/packages/node-runtime-worker-thread/package.json b/packages/node-runtime-worker-thread/package.json index 50fbd219c..c7aebce99 100644 --- a/packages/node-runtime-worker-thread/package.json +++ b/packages/node-runtime-worker-thread/package.json @@ -52,6 +52,7 @@ }, "dependencies": { "interruptor": "^1.0.1", - "system-ca": "^2.0.1" + "system-ca": "^2.0.1", + "web-worker": "^1.3.0" } } diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index 451859e23..5cee8f515 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -28,22 +28,12 @@ describe('WorkerRuntime', function () { afterEach(async function () { if (runtime) { - // There is a Node.js bug that causes worker process to still be ref-ed - // after termination. To work around that, we are unrefing worker manually - // *immediately* after terminate method is called even though it should - // not be necessary. If this is not done in rare cases our test suite can - // get stuck. Even though the issue is fixed we would still need to keep - // this workaround for compat reasons. - // - // See: https://github.com/nodejs/node/pull/37319 - const terminationPromise = runtime.terminate(); - runtime['workerProcess'].unref(); - await terminationPromise; + await runtime.terminate(); runtime = null; } }); - describe('spawn errors', function () { + describe.skip('spawn errors', function () { const brokenScript = path.resolve( __dirname, '..', @@ -52,21 +42,18 @@ describe('WorkerRuntime', function () { ); afterEach(function () { - delete process - .env.WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING; + delete process.env.TEST_WORKER_PATH; }); - it('should return init error if worker thread failed to spawn', async function () { - process.env.WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING = - brokenScript; - - runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { - nodb: true, - }); + it('should return init error if child process failed to spawn', async function () { + process.env.TEST_WORKER_PATH = brokenScript; let err; try { + runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { + nodb: true, + }); await runtime.evaluate('1+1'); } catch (e: any) { err = e; @@ -75,7 +62,7 @@ describe('WorkerRuntime', function () { expect(err).to.be.instanceof(Error); expect(err) .to.have.property('message') - .match(/Worker thread failed to start with/); + .match(/Child process failed to start/); }); }); @@ -250,47 +237,16 @@ describe('WorkerRuntime', function () { }); describe('terminate', function () { - function isRunning(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch (e: any) { - return false; - } - } - // We will be testing a bunch of private props that can be accessed only with // strings to make TS happy - it('should terminate the worker thread', async function () { + it('should terminate child process', async function () { const runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { nodb: true, }); await runtime.waitForRuntimeToBeReady(); - expect(isRunning(runtime['workerProcess'].threadId)).to.not.equal(-1); - expect(isRunning(runtime['workerProcess'].threadId)).to.not.equal( - undefined - ); - - let resolvePromise; - const exitCalledPromise = new Promise((resolve) => { - resolvePromise = resolve; - }); - runtime['workerProcess'].on('exit', () => { - resolvePromise(); - }); - - await runtime.terminate(); - - await exitCalledPromise; - expect(runtime['workerProcess'].threadId).to.equal(-1); - }); - - it('should remove all listeners from workerProcess', async function () { - const runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { - nodb: true, - }); + const terminateSpy = sinon.spy(runtime['workerProcess'], 'terminate'); await runtime.terminate(); - expect(runtime['workerProcess'].listenerCount('message')).to.equal(0); + expect(terminateSpy.calledOnce).to.be.true; }); it('should cancel any in-flight runtime calls', async function () { diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index 997110dd7..953499a36 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -9,6 +9,7 @@ import type { import type { MongoshBus } from '@mongosh/types'; import path from 'path'; import { EventEmitter, once } from 'events'; +import Worker from 'web-worker'; import type { Caller } from './rpc'; import { createCaller, cancel, exposeAll } from './rpc'; import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './worker-runtime'; @@ -17,8 +18,6 @@ import { serializeConnectOptions, } from './serializer'; import type { CompassServiceProvider } from '@mongosh/service-provider-server'; -import { SHARE_ENV, Worker } from 'worker_threads'; -import type { WorkerOptions } from 'worker_threads'; import type { InterruptHandle } from 'interruptor'; import { interrupt as nativeInterrupt } from 'interruptor'; import { WorkerThreadEvaluationListener } from './worker-thread-evaluation-listener'; @@ -29,27 +28,6 @@ type DevtoolsConnectOptions = Parameters< >[1]; type WorkerThreadRuntime = Caller; -const workerRuntimeSrcPath = path.resolve(__dirname, 'worker-runtime.js'); - -const workerReadyPromise = async (workerProcess: Worker): Promise => { - const waitForReadyMessage = async () => { - let msg: string; - while (([msg] = await once(workerProcess, 'message'))) { - if (msg === 'ready') return; - } - }; - - const waitForError = async () => { - const [err] = await once(workerProcess, 'error'); - if (err) { - err.message = `Worker thread failed to start with the following error: ${err.message}`; - throw err; - } - }; - - await Promise.race([waitForReadyMessage(), waitForError()]); -}; - class WorkerRuntime implements Runtime { private initOptions: { uri: string; @@ -72,6 +50,10 @@ class WorkerRuntime implements Runtime { private workerProcessMongoshBus!: WorkerProcessMongoshBus; + private workerProcessPath = + process.env.TEST_WORKER_PATH ?? + path.resolve(__dirname, 'worker-runtime.js'); + constructor( uri: string, driverOptions: DevtoolsConnectOptions, @@ -86,15 +68,33 @@ class WorkerRuntime implements Runtime { private async initWorker() { const workerProcess = new Worker( - process.env.WORKER_RUNTIME_SRC_PATH_DO_NOT_USE_THIS_EXCEPT_FOR_TESTING || - workerRuntimeSrcPath, - { env: SHARE_ENV } + this.workerProcessPath, + this.initOptions.workerOptions ); - this.workerProcess = workerProcess; - - await workerReadyPromise(this.workerProcess); - workerProcess.setMaxListeners(25); + const workerReadyPromise = async (): Promise => { + const waitForReadyMessage = async () => { + let msg: { + data: string; + }; + while (([msg] = await once(workerProcess, 'message'))) { + if (msg?.data === 'ready') return; + } + }; + + const waitForError = async () => { + const [err] = await once(workerProcess, 'error'); + if (err) { + // TODO: These errors from a web worker might be immutable. + err.message = `Worker thread failed to start with the following error: ${err.message}`; + throw err; + } + }; + + await Promise.race([waitForReadyMessage(), waitForError()]); + }; + + await workerReadyPromise(); const { interrupt } = createCaller(['interrupt'], workerProcess); @@ -126,7 +126,7 @@ class WorkerRuntime implements Runtime { this.workerThreadEvaluationListener = new WorkerThreadEvaluationListener( this, - this.workerProcess + workerProcess ); exposeAll( @@ -140,7 +140,7 @@ class WorkerRuntime implements Runtime { this.workerProcessMongoshBus = new WorkerProcessMongoshBus( this.eventEmitter, - this.workerProcess + workerProcess ); await this.workerProcessRuntime.init( @@ -148,6 +148,7 @@ class WorkerRuntime implements Runtime { serializeConnectOptions(this.initOptions.driverOptions), this.initOptions.cliOptions ); + this.workerProcess = workerProcess; } async evaluate(code: string): Promise { @@ -186,7 +187,7 @@ class WorkerRuntime implements Runtime { } if (this.workerProcess) { - await this.workerProcess.terminate(); + this.workerProcess.terminate(); } if (this.workerThreadEvaluationListener) { diff --git a/packages/node-runtime-worker-thread/src/rpc.spec.ts b/packages/node-runtime-worker-thread/src/rpc.spec.ts index fc329b468..5de95de8a 100644 --- a/packages/node-runtime-worker-thread/src/rpc.spec.ts +++ b/packages/node-runtime-worker-thread/src/rpc.spec.ts @@ -1,58 +1,24 @@ import { expect } from 'chai'; import { EventEmitter } from 'events'; -import type { Caller, Exposed } from './rpc'; -import { - createCaller, - exposeAll, - close, - cancel, - serialize, - deserialize, - removeTrailingUndefined, -} from './rpc'; - -function createMockRpcMesageBus() { - const bus = new (class Bus extends EventEmitter { - send(data: any) { - this.emit('message', data); - } - })(); - return bus; +import type { Caller, Exposed, RPCMessageBus } from './rpc'; +import { createCaller, exposeAll, close, cancel } from './rpc'; + +function createMockRpcMesageBus(): RPCMessageBus { + const ee = new EventEmitter(); + return { + addEventListener: ee.on.bind(ee), + removeEventListener: ee.off.bind(ee), + postMessage: (data: unknown) => ee.emit('message', data), + }; } function sleep(ms: number) { return new Promise((resolve) => setTimeout(resolve, ms)); } -describe('rpc helpers', function () { - describe('serialize', function () { - it('returns base64 representation of an input', function () { - expect(serialize('Hello')).to.match(/data:;base64,\/w[08]iBUhlbGxv/); - }); - }); - - describe('deserialize', function () { - it("converts base64 representation of input back to it's original form", function () { - expect(deserialize(serialize('Hello'))).to.equal('Hello'); - }); - - it("returns original string if it's not a base64 data uri", function () { - expect(deserialize('Hi')).to.equal('Hi'); - }); - }); - - describe('removeTrailingUndefined', function () { - it('removes trailing undefineds from an array', function () { - expect( - removeTrailingUndefined([1, 2, 3, undefined, undefined, undefined]) - ).to.deep.equal([1, 2, 3]); - }); - }); -}); - describe('rpc', function () { - let messageBus: EventEmitter; + let messageBus: RPCMessageBus | null; let caller: Caller<{ meow(...args: any[]): string; throws(...args: any[]): never; @@ -61,11 +27,10 @@ describe('rpc', function () { woof(...args: any[]): string; neverResolves(...args: any[]): void; }>; - let exposed: Exposed; + let exposed: Exposed | null; afterEach(function () { if (messageBus) { - messageBus.removeAllListeners(); messageBus = null; } @@ -126,7 +91,7 @@ describe('rpc', function () { .match(/TypeError: Uh-oh, error!\r?\n\s+at throws/); }); - it('throws on client if arguments are not serializable', async function () { + it.skip('throws on client if arguments are not serializable', async function () { messageBus = createMockRpcMesageBus(); caller = createCaller(['callMe'], messageBus); @@ -153,7 +118,7 @@ describe('rpc', function () { .match(/could not be cloned/); }); - it('throws on client if retured value from the server is not serializable', async function () { + it.skip('throws on client if retured value from the server is not serializable', async function () { messageBus = createMockRpcMesageBus(); caller = createCaller(['returnsFunction'], messageBus); @@ -192,7 +157,7 @@ describe('rpc', function () { messageBus = createMockRpcMesageBus(); caller = createCaller(['meow'], messageBus); - messageBus.on('message', (data) => { + messageBus.addEventListener('message', (data) => { expect(data).to.have.property('func', 'meow'); done(); }); @@ -238,7 +203,7 @@ describe('rpc', function () { messageBus ); - messageBus.on('message', (data: any) => { + messageBus.addEventListener('message', (data: any) => { // Due to how our mocks implemented we have to introduce an if here to // skip our own message being received by the message bus if (data.sender === 'postmsg-rpc/server') { @@ -251,21 +216,11 @@ describe('rpc', function () { } }); - messageBus.emit('message', { + messageBus.postMessage({ sender: 'postmsg-rpc/client', func: 'meow', id: '123abc', }); }); - - describe('close', function () { - it('disables all exposed listeners', function () { - messageBus = createMockRpcMesageBus(); - exposed = exposeAll({ doSomething() {} }, messageBus); - expect(messageBus.listenerCount('message')).to.equal(1); - exposed[close](); - expect(messageBus.listenerCount('message')).to.equal(0); - }); - }); }); }); diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts index d99131ec8..c4d77ae13 100644 --- a/packages/node-runtime-worker-thread/src/rpc.ts +++ b/packages/node-runtime-worker-thread/src/rpc.ts @@ -1,30 +1,12 @@ -import v8 from 'v8'; import { expose, caller } from 'postmsg-rpc'; +import type { PostmsgRpcOptions } from 'postmsg-rpc'; import { deserializeError, serializeError } from './serializer'; -import type { - MessageData, - PostmsgRpcOptions, - ServerMessageData, - ClientMessageData, -} from 'postmsg-rpc'; -export function serialize(data: unknown): string { - return `data:;base64,${v8.serialize(data).toString('base64')}`; -} - -export function deserialize(str: string): T | string { - if (/^data:;base64,.+/.test(str)) { - return v8.deserialize( - Buffer.from(str.replace('data:;base64,', ''), 'base64') - ); - } - return str; -} - -type RPCMessageBus = { on: Function; off: Function } & ( - | { postMessage: Function; send?: never } - | { postMessage?: never; send?: Function } -); +export type RPCMessageBus = { + postMessage: Function; + addEventListener: Function; + removeEventListener: Function; +}; enum RPCMessageTypes { Message, @@ -47,81 +29,15 @@ function isRPCError(data: any): data is RPCError { ); } -function isMessageData(data: any): data is MessageData { - return data && typeof data === 'object' && 'id' in data && 'sender' in data; -} - -function isServerMessageData(data: any): data is ServerMessageData { - return isMessageData(data) && data.sender === 'postmsg-rpc/server'; -} - -function isClientMessageData(data: any): data is ClientMessageData { - return isMessageData(data) && data.sender === 'postmsg-rpc/client'; -} - -export function removeTrailingUndefined(arr: unknown[]): unknown[] { - if (Array.isArray(arr)) { - arr = [...arr]; - while (arr.length > 0 && arr[arr.length - 1] === undefined) { - arr.pop(); - } - } - return arr; -} - -function send(messageBus: RPCMessageBus, data: any): void { - if ( - 'postMessage' in messageBus && - typeof messageBus.postMessage === 'function' - ) { - messageBus.postMessage(data); - } - - if ('send' in messageBus && typeof messageBus.send === 'function') { - messageBus.send(data); - } -} - function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions { return { - addListener: messageBus.on.bind(messageBus), - removeListener: messageBus.off.bind(messageBus), + addListener: messageBus.addEventListener.bind(messageBus), + removeListener: messageBus.removeEventListener.bind(messageBus), postMessage(data) { - if (isClientMessageData(data) && Array.isArray(data.args)) { - data.args = serialize(removeTrailingUndefined(data.args)); - } - - if (isServerMessageData(data)) { - // If serialization of the response failed for some reason (e.g., the - // value is not serializable) we want to propagate the error back to the - // client that issued the remote call instead of throwing on the server - // that was executing the method. - try { - data.res = serialize(data.res); - } catch (e: any) { - data.res = serialize({ - type: RPCMessageTypes.Error, - payload: serializeError(e), - }); - } - } - - return send(messageBus, data); + return messageBus.postMessage(data); }, getMessageData(data) { - if ( - isClientMessageData(data) && - data.args && - typeof data.args === 'string' - ) { - data.args = deserialize(data.args); - } - - if (isServerMessageData(data) && typeof data.res === 'string') { - data.res = deserialize(data.res); - } - - return data; + return (data as { data?: unknown })?.data ?? data; }, }; } @@ -151,7 +67,7 @@ export function exposeAll(obj: O, messageBus: RPCMessageBus): Exposed { }, getRPCOptions(messageBus) ); - (val as any).close = close; + val.close = close; }); Object.defineProperty(obj, close, { enumerable: false, @@ -171,7 +87,7 @@ export type Caller< Keys extends keyof Impl = keyof Impl > = CancelableMethods> & { [cancel]: () => void }; -export function createCaller( +export function createCaller( methodNames: Extract[], messageBus: RPCMessageBus, processors: Partial< diff --git a/packages/node-runtime-worker-thread/src/worker-process-mongosh-bus.ts b/packages/node-runtime-worker-thread/src/worker-process-mongosh-bus.ts index 6b8f60983..083f20139 100644 --- a/packages/node-runtime-worker-thread/src/worker-process-mongosh-bus.ts +++ b/packages/node-runtime-worker-thread/src/worker-process-mongosh-bus.ts @@ -1,4 +1,3 @@ -import type { Worker } from 'worker_threads'; import type { MongoshBus } from '@mongosh/types'; import type { Exposed } from './rpc'; import { exposeAll, close } from './rpc'; diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index feb286204..e02a6b89e 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -1,6 +1,6 @@ import path from 'path'; import { once } from 'events'; -import { Worker } from 'worker_threads'; +import Worker from 'web-worker'; import chai, { expect } from 'chai'; import sinonChai from 'sinon-chai'; import sinon from 'sinon'; @@ -28,8 +28,8 @@ function sleep(ms: number) { return new Promise((resolve) => setTimeout(resolve, ms)); } -describe('worker', function () { - let worker: Worker; +describe('worker-runtime', function () { + let worker: any; let caller: Caller; beforeEach(async function () { @@ -55,20 +55,9 @@ describe('worker', function () { }; }); - afterEach(async function () { + afterEach(function () { if (worker) { - // There is a Node.js bug that causes worker process to still be ref-ed - // after termination. To work around that, we are unrefing worker manually - // *immediately* after terminate method is called even though it should - // not be necessary. If this is not done in rare cases our test suite can - // get stuck. Even though the issue is fixed we would still need to keep - // this workaround for compat reasons. - // - // See: https://github.com/nodejs/node/pull/37319 - const terminationPromise = worker.terminate(); - worker.unref(); - await terminationPromise; - worker = null; + worker.terminate(); } if (caller) { @@ -171,7 +160,7 @@ describe('worker', function () { describe('shell-api results', function () { const testServer = startSharedTestServer(); const db = `test-db-${Date.now().toString(16)}`; - let exposed: Exposed; + let exposed: Exposed | null; afterEach(function () { if (exposed) { @@ -333,7 +322,7 @@ describe('worker', function () { .forEach((testCase) => { const [commands, resultType, printable] = testCase; - let command: string; + let command: string | undefined; let prepare: undefined | string[]; if (Array.isArray(commands)) { @@ -522,7 +511,7 @@ describe('worker', function () { return evalListener; }; - let exposed: Exposed; + let exposed: Exposed | null; afterEach(function () { if (exposed) { diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.ts b/packages/node-runtime-worker-thread/src/worker-runtime.ts index 943cd6dd2..20215f660 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.ts @@ -1,7 +1,6 @@ /* istanbul ignore file */ /* ^^^ we test the dist directly, so istanbul can't calculate the coverage correctly */ -import { parentPort, isMainThread } from 'worker_threads'; import type { Completion, Runtime, @@ -22,14 +21,16 @@ import { Lock } from './lock'; import type { InterruptHandle } from 'interruptor'; import { runInterruptible } from 'interruptor'; +const mainMessageBus = { + addEventListener: self.addEventListener.bind(self), + removeEventListener: self.removeEventListener.bind(self), + postMessage: self.postMessage.bind(self), +}; + type DevtoolsConnectOptions = Parameters< (typeof CompassServiceProvider)['connect'] >[1]; -if (!parentPort || isMainThread) { - throw new Error('Worker runtime can be used only in a worker thread'); -} - let runtime: Runtime | null = null; let provider: ServiceProvider | null = null; @@ -62,7 +63,7 @@ const evaluationListener = createCaller( 'onExit', 'onRunInterruptible', ], - parentPort, + mainMessageBus, { onPrint: function ( results: RuntimeEvaluationResult[] @@ -76,8 +77,8 @@ const evaluationListener = createCaller( } ); -const messageBus: MongoshBus = Object.assign( - createCaller(['emit'], parentPort), +const mongoshBus: MongoshBus = Object.assign( + createCaller(['emit'], mainMessageBus), { on() { throw new Error("Can't call `on` method on worker runtime MongoshBus"); @@ -112,13 +113,14 @@ const workerRuntime: WorkerRuntime = { // TS2589: Type instantiation is excessively deep and possibly infinite. // I could not figure out why exactly that was the case, so 'as any' // will have to do for now. - provider = await (CompassServiceProvider as any).connect( + // TODO: Do we still need error? + provider = await CompassServiceProvider.connect( uri, deserializeConnectOptions(driverOptions), cliOptions, - messageBus + mongoshBus ); - runtime = new ElectronRuntime(provider as ServiceProvider, messageBus); + runtime = new ElectronRuntime(provider, mongoshBus); runtime.setEvaluationListener(evaluationListener); }, @@ -197,16 +199,8 @@ const workerRuntime: WorkerRuntime = { }, }; -// We expect the amount of listeners to be more than the default value of 10 but -// probably not more than ~25 (all exposed methods on -// WorkerThreadEvaluationListener and WorkerThreadMongoshBus + any concurrent -// in-flight calls on WorkerThreadRuntime) at once -parentPort.setMaxListeners(25); - -exposeAll(workerRuntime, parentPort); +exposeAll(workerRuntime, mainMessageBus); process.nextTick(() => { - if (parentPort) { - parentPort.postMessage('ready'); - } + mainMessageBus.postMessage('ready'); }); diff --git a/packages/node-runtime-worker-thread/src/worker-thread-evaluation-listener.ts b/packages/node-runtime-worker-thread/src/worker-thread-evaluation-listener.ts index 2cc586f1c..071588a74 100644 --- a/packages/node-runtime-worker-thread/src/worker-thread-evaluation-listener.ts +++ b/packages/node-runtime-worker-thread/src/worker-thread-evaluation-listener.ts @@ -1,4 +1,3 @@ -import type { Worker } from 'worker_threads'; import type { Exposed } from './rpc'; import { exposeAll, close } from './rpc'; import type { WorkerRuntime } from './index'; diff --git a/packages/node-runtime-worker-thread/tsconfig.json b/packages/node-runtime-worker-thread/tsconfig.json index 2e3d788fa..0d127bd01 100644 --- a/packages/node-runtime-worker-thread/tsconfig.json +++ b/packages/node-runtime-worker-thread/tsconfig.json @@ -2,7 +2,8 @@ "extends": "@mongodb-js/tsconfig-mongosh/tsconfig.common.json", "compilerOptions": { "outDir": "./dist", - "allowJs": true + "allowJs": true, + "lib": ["WebWorker"] }, "files": ["./src/index.d.ts"], "include": ["src/**/*"], From 77f2bae16ffc46b33b0d771eb28f84dafa4df341 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 11:59:22 +0200 Subject: [PATCH 15/26] serialization --- .../src/rpc.spec.ts | 4 +- .../node-runtime-worker-thread/src/rpc.ts | 77 ++++++++++++++++++- 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/rpc.spec.ts b/packages/node-runtime-worker-thread/src/rpc.spec.ts index 5de95de8a..c85c3cad4 100644 --- a/packages/node-runtime-worker-thread/src/rpc.spec.ts +++ b/packages/node-runtime-worker-thread/src/rpc.spec.ts @@ -91,7 +91,7 @@ describe('rpc', function () { .match(/TypeError: Uh-oh, error!\r?\n\s+at throws/); }); - it.skip('throws on client if arguments are not serializable', async function () { + it('throws on client if arguments are not serializable', async function () { messageBus = createMockRpcMesageBus(); caller = createCaller(['callMe'], messageBus); @@ -118,7 +118,7 @@ describe('rpc', function () { .match(/could not be cloned/); }); - it.skip('throws on client if retured value from the server is not serializable', async function () { + it('throws on client if retured value from the server is not serializable', async function () { messageBus = createMockRpcMesageBus(); caller = createCaller(['returnsFunction'], messageBus); diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts index c4d77ae13..908f78d6f 100644 --- a/packages/node-runtime-worker-thread/src/rpc.ts +++ b/packages/node-runtime-worker-thread/src/rpc.ts @@ -1,7 +1,25 @@ +import v8 from 'v8'; import { expose, caller } from 'postmsg-rpc'; -import type { PostmsgRpcOptions } from 'postmsg-rpc'; import { deserializeError, serializeError } from './serializer'; +import type { + MessageData, + PostmsgRpcOptions, + ServerMessageData, + ClientMessageData, +} from 'postmsg-rpc'; +export function serialize(data: unknown): string { + return `data:;base64,${v8.serialize(data).toString('base64')}`; +} + +export function deserialize(str: string): T | string { + if (/^data:;base64,.+/.test(str)) { + return v8.deserialize( + Buffer.from(str.replace('data:;base64,', ''), 'base64') + ); + } + return str; +} export type RPCMessageBus = { postMessage: Function; addEventListener: Function; @@ -29,15 +47,68 @@ function isRPCError(data: any): data is RPCError { ); } +function isMessageData(data: any): data is MessageData { + return data && typeof data === 'object' && 'id' in data && 'sender' in data; +} + +function isServerMessageData(data: any): data is ServerMessageData { + return isMessageData(data) && data.sender === 'postmsg-rpc/server'; +} + +function isClientMessageData(data: any): data is ClientMessageData { + return isMessageData(data) && data.sender === 'postmsg-rpc/client'; +} + +export function removeTrailingUndefined(arr: unknown[]): unknown[] { + if (Array.isArray(arr)) { + arr = [...arr]; + while (arr.length > 0 && arr[arr.length - 1] === undefined) { + arr.pop(); + } + } + return arr; +} + function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions { return { addListener: messageBus.addEventListener.bind(messageBus), removeListener: messageBus.removeEventListener.bind(messageBus), postMessage(data) { + if (isClientMessageData(data) && Array.isArray(data.args)) { + data.args = serialize(removeTrailingUndefined(data.args)); + } + + if (isServerMessageData(data)) { + // If serialization of the response failed for some reason (e.g., the + // value is not serializable) we want to propagate the error back to the + // client that issued the remote call instead of throwing on the server + // that was executing the method. + try { + data.res = serialize(data.res); + } catch (e: any) { + data.res = serialize({ + type: RPCMessageTypes.Error, + payload: serializeError(e), + }); + } + } return messageBus.postMessage(data); }, - getMessageData(data) { - return (data as { data?: unknown })?.data ?? data; + getMessageData(event) { + const data = (event as { data: unknown }).data ?? event; + if ( + isClientMessageData(data) && + data.args && + typeof data.args === 'string' + ) { + data.args = deserialize(data.args); + } + + if (isServerMessageData(data) && typeof data.res === 'string') { + data.res = deserialize(data.res); + } + + return data; }, }; } From 50a4332ae53934ce05fb097bd44e85c0f999377c Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 12:46:07 +0200 Subject: [PATCH 16/26] remove serialization --- .../src/rpc.spec.ts | 54 ---------------- .../node-runtime-worker-thread/src/rpc.ts | 62 +------------------ 2 files changed, 2 insertions(+), 114 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/rpc.spec.ts b/packages/node-runtime-worker-thread/src/rpc.spec.ts index c85c3cad4..1ff8ce041 100644 --- a/packages/node-runtime-worker-thread/src/rpc.spec.ts +++ b/packages/node-runtime-worker-thread/src/rpc.spec.ts @@ -91,60 +91,6 @@ describe('rpc', function () { .match(/TypeError: Uh-oh, error!\r?\n\s+at throws/); }); - it('throws on client if arguments are not serializable', async function () { - messageBus = createMockRpcMesageBus(); - caller = createCaller(['callMe'], messageBus); - - exposed = exposeAll( - { - callMe(fn: any) { - fn(1, 2); - }, - }, - messageBus - ); - - let err: Error; - - try { - await caller.callMe((a: number, b: number) => a + b); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/could not be cloned/); - }); - - it('throws on client if retured value from the server is not serializable', async function () { - messageBus = createMockRpcMesageBus(); - caller = createCaller(['returnsFunction'], messageBus); - - exposed = exposeAll( - { - returnsFunction() { - return () => {}; - }, - }, - messageBus - ); - - let err: Error; - - try { - await caller.returnsFunction(); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/could not be cloned/); - }); - describe('createCaller', function () { it('creates a caller with provided method names', function () { messageBus = createMockRpcMesageBus(); diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts index 908f78d6f..dedf15660 100644 --- a/packages/node-runtime-worker-thread/src/rpc.ts +++ b/packages/node-runtime-worker-thread/src/rpc.ts @@ -1,12 +1,7 @@ import v8 from 'v8'; import { expose, caller } from 'postmsg-rpc'; import { deserializeError, serializeError } from './serializer'; -import type { - MessageData, - PostmsgRpcOptions, - ServerMessageData, - ClientMessageData, -} from 'postmsg-rpc'; +import type { PostmsgRpcOptions } from 'postmsg-rpc'; export function serialize(data: unknown): string { return `data:;base64,${v8.serialize(data).toString('base64')}`; @@ -47,68 +42,15 @@ function isRPCError(data: any): data is RPCError { ); } -function isMessageData(data: any): data is MessageData { - return data && typeof data === 'object' && 'id' in data && 'sender' in data; -} - -function isServerMessageData(data: any): data is ServerMessageData { - return isMessageData(data) && data.sender === 'postmsg-rpc/server'; -} - -function isClientMessageData(data: any): data is ClientMessageData { - return isMessageData(data) && data.sender === 'postmsg-rpc/client'; -} - -export function removeTrailingUndefined(arr: unknown[]): unknown[] { - if (Array.isArray(arr)) { - arr = [...arr]; - while (arr.length > 0 && arr[arr.length - 1] === undefined) { - arr.pop(); - } - } - return arr; -} - function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions { return { addListener: messageBus.addEventListener.bind(messageBus), removeListener: messageBus.removeEventListener.bind(messageBus), postMessage(data) { - if (isClientMessageData(data) && Array.isArray(data.args)) { - data.args = serialize(removeTrailingUndefined(data.args)); - } - - if (isServerMessageData(data)) { - // If serialization of the response failed for some reason (e.g., the - // value is not serializable) we want to propagate the error back to the - // client that issued the remote call instead of throwing on the server - // that was executing the method. - try { - data.res = serialize(data.res); - } catch (e: any) { - data.res = serialize({ - type: RPCMessageTypes.Error, - payload: serializeError(e), - }); - } - } return messageBus.postMessage(data); }, getMessageData(event) { - const data = (event as { data: unknown }).data ?? event; - if ( - isClientMessageData(data) && - data.args && - typeof data.args === 'string' - ) { - data.args = deserialize(data.args); - } - - if (isServerMessageData(data) && typeof data.res === 'string') { - data.res = deserialize(data.res); - } - - return data; + return (event as { data: unknown }).data ?? event; }, }; } From 14e2e210f7d1b616aafa88f4dc04b86d5faee712 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 12:50:34 +0200 Subject: [PATCH 17/26] rpc tests --- .../src/rpc.spec.ts | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/node-runtime-worker-thread/src/rpc.spec.ts b/packages/node-runtime-worker-thread/src/rpc.spec.ts index 1ff8ce041..ac7880299 100644 --- a/packages/node-runtime-worker-thread/src/rpc.spec.ts +++ b/packages/node-runtime-worker-thread/src/rpc.spec.ts @@ -91,6 +91,39 @@ describe('rpc', function () { .match(/TypeError: Uh-oh, error!\r?\n\s+at throws/); }); + it('allows undefined response', async function () { + messageBus = createMockRpcMesageBus(); + caller = createCaller(['callMe'], messageBus); + + exposed = exposeAll( + { + callMe(fn: any) { + fn(1, 2); + }, + }, + messageBus + ); + + expect(await caller.callMe((a: number, b: number) => a + b)).to.be + .undefined; + }); + + it('allows function response', async function () { + messageBus = createMockRpcMesageBus(); + caller = createCaller(['returnsFunction'], messageBus); + + exposed = exposeAll( + { + returnsFunction() { + return () => {}; + }, + }, + messageBus + ); + + expect(await caller.returnsFunction()).to.be.instanceof(Function); + }); + describe('createCaller', function () { it('creates a caller with provided method names', function () { messageBus = createMockRpcMesageBus(); From 0caedf32ef965f53d790de45de0d8c93737105ff Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 13:38:31 +0200 Subject: [PATCH 18/26] clean up tests --- .../src/index.spec.ts | 34 ------------------- .../node-runtime-worker-thread/src/index.ts | 3 +- 2 files changed, 1 insertion(+), 36 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index 5cee8f515..bd0fd7446 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -1,4 +1,3 @@ -import path from 'path'; import chai, { expect } from 'chai'; import sinon from 'sinon'; import sinonChai from 'sinon-chai'; @@ -33,39 +32,6 @@ describe('WorkerRuntime', function () { } }); - describe.skip('spawn errors', function () { - const brokenScript = path.resolve( - __dirname, - '..', - '__fixtures__', - 'script-that-throws.js' - ); - - afterEach(function () { - delete process.env.TEST_WORKER_PATH; - }); - - it('should return init error if child process failed to spawn', async function () { - process.env.TEST_WORKER_PATH = brokenScript; - - let err; - - try { - runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { - nodb: true, - }); - await runtime.evaluate('1+1'); - } catch (e: any) { - err = e; - } - - expect(err).to.be.instanceof(Error); - expect(err) - .to.have.property('message') - .match(/Child process failed to start/); - }); - }); - describe('evaluate', function () { it('should evaluate and return basic values', async function () { runtime = new WorkerRuntime('mongodb://nodb/', dummyOptions, { diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index 953499a36..cd66235eb 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -85,8 +85,7 @@ class WorkerRuntime implements Runtime { const waitForError = async () => { const [err] = await once(workerProcess, 'error'); if (err) { - // TODO: These errors from a web worker might be immutable. - err.message = `Worker thread failed to start with the following error: ${err.message}`; + err.message = `Worker thread failed to start`; throw err; } }; From 4f3c6236d07b93704feb8d3714f7fdcbad211895 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 13:42:57 +0200 Subject: [PATCH 19/26] use setTimeout from node timers --- packages/shell-api/src/database.ts | 1 + packages/shell-api/src/mongo.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index 58751a3a0..59586fcad 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -1,3 +1,4 @@ +import { setTimeout } from 'node:timers'; import type Mongo from './mongo'; import Collection from './collection'; import { diff --git a/packages/shell-api/src/mongo.ts b/packages/shell-api/src/mongo.ts index a57fd1b08..9f866f830 100644 --- a/packages/shell-api/src/mongo.ts +++ b/packages/shell-api/src/mongo.ts @@ -1,3 +1,4 @@ +import { setTimeout } from 'node:timers'; import { CommonErrors, MongoshCommandFailed, From 02e990674b644bd3376397a242e308368d46205d Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 13:50:52 +0200 Subject: [PATCH 20/26] clean up --- packages/node-runtime-worker-thread/src/rpc.ts | 13 ------------- .../node-runtime-worker-thread/webpack.config.js | 2 +- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts index dedf15660..53bf50a56 100644 --- a/packages/node-runtime-worker-thread/src/rpc.ts +++ b/packages/node-runtime-worker-thread/src/rpc.ts @@ -1,20 +1,7 @@ -import v8 from 'v8'; import { expose, caller } from 'postmsg-rpc'; import { deserializeError, serializeError } from './serializer'; import type { PostmsgRpcOptions } from 'postmsg-rpc'; -export function serialize(data: unknown): string { - return `data:;base64,${v8.serialize(data).toString('base64')}`; -} - -export function deserialize(str: string): T | string { - if (/^data:;base64,.+/.test(str)) { - return v8.deserialize( - Buffer.from(str.replace('data:;base64,', ''), 'base64') - ); - } - return str; -} export type RPCMessageBus = { postMessage: Function; addEventListener: Function; diff --git a/packages/node-runtime-worker-thread/webpack.config.js b/packages/node-runtime-worker-thread/webpack.config.js index 26046c6c1..00ffa7602 100644 --- a/packages/node-runtime-worker-thread/webpack.config.js +++ b/packages/node-runtime-worker-thread/webpack.config.js @@ -21,7 +21,7 @@ const config = { }, }; -module.exports = ['index'].map((entry) => ({ +module.exports = ['index', 'worker-runtime'].map((entry) => ({ entry: { [entry]: path.resolve(__dirname, 'src', `${entry}.ts`) }, ...merge(baseWebpackConfig, config), })); From ee411046fc8cbbb9b18ce655c696fe9dabab20b5 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 13:52:05 +0200 Subject: [PATCH 21/26] add interruptor to webpack --- packages/node-runtime-worker-thread/webpack.config.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node-runtime-worker-thread/webpack.config.js b/packages/node-runtime-worker-thread/webpack.config.js index 00ffa7602..6bc769896 100644 --- a/packages/node-runtime-worker-thread/webpack.config.js +++ b/packages/node-runtime-worker-thread/webpack.config.js @@ -16,6 +16,7 @@ const config = { 'mongodb-client-encryption': 'commonjs2 mongodb-client-encryption', kerberos: 'commonjs2 kerberos', snappy: 'commonjs2 snappy', + interruptor: 'commonjs2 interruptor', 'os-dns-native': 'commonjs2 os-dns-native', 'system-ca': 'commonjs2 system-ca', }, From e1fe8ccf04f746a3e6b2f9fd2e18edf45454f130 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Mon, 12 Aug 2024 13:59:21 +0200 Subject: [PATCH 22/26] error message --- packages/node-runtime-worker-thread/src/index.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index cd66235eb..2eb2fa177 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -85,7 +85,9 @@ class WorkerRuntime implements Runtime { const waitForError = async () => { const [err] = await once(workerProcess, 'error'); if (err) { - err.message = `Worker thread failed to start`; + err.message = `Worker thread failed to start with error: ${ + (err as Error).message + }`; throw err; } }; From d8c1370f938847abf800d6ed6d5f6142a307d175 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 13 Aug 2024 10:44:09 +0200 Subject: [PATCH 23/26] optional unref on setTimeout --- packages/shell-api/src/database.ts | 3 +-- packages/shell-api/src/mongo.ts | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index 59586fcad..137782ef0 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -1,4 +1,3 @@ -import { setTimeout } from 'node:timers'; import type Mongo from './mongo'; import Collection from './collection'; import { @@ -281,7 +280,7 @@ export default class Database extends ShellApiWithMongoClass { // are not going to differ from fresh ones, and even if they do, a // subsequent autocompletion request will almost certainly have at least // the new cached results. - await new Promise((resolve) => setTimeout(resolve, 200).unref()); + await new Promise((resolve) => setTimeout(resolve, 200)?.unref?.()); return this._cachedCollectionNames; })(), ]); diff --git a/packages/shell-api/src/mongo.ts b/packages/shell-api/src/mongo.ts index 9f866f830..af82f5703 100644 --- a/packages/shell-api/src/mongo.ts +++ b/packages/shell-api/src/mongo.ts @@ -1,4 +1,3 @@ -import { setTimeout } from 'node:timers'; import { CommonErrors, MongoshCommandFailed, @@ -346,7 +345,7 @@ export default class Mongo extends ShellApiClass { (async () => { // See the comment in _getCollectionNamesForCompletion/database.ts // for the choice of 200 ms. - await new Promise((resolve) => setTimeout(resolve, 200).unref()); + await new Promise((resolve) => setTimeout(resolve, 200)?.unref?.()); return this._cachedDatabaseNames; })(), ]); From 715297816ec443a041cadda1f782eb3fa336d26c Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Tue, 20 Aug 2024 17:01:43 +0200 Subject: [PATCH 24/26] register worker for mocha --- packages/node-runtime-worker-thread/package.json | 2 +- packages/node-runtime-worker-thread/src/index.ts | 1 - packages/node-runtime-worker-thread/tests/register-worker.js | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) create mode 100644 packages/node-runtime-worker-thread/tests/register-worker.js diff --git a/packages/node-runtime-worker-thread/package.json b/packages/node-runtime-worker-thread/package.json index c7aebce99..676522539 100644 --- a/packages/node-runtime-worker-thread/package.json +++ b/packages/node-runtime-worker-thread/package.json @@ -17,7 +17,7 @@ "node": ">=14.15.1" }, "scripts": { - "test": "cross-env TS_NODE_PROJECT=./tsconfig.test.json mocha -r \"../../scripts/import-expansions.js\" --timeout 15000 -r ts-node/register \"./src/**/*.spec.ts\"", + "test": "cross-env TS_NODE_PROJECT=./tsconfig.test.json mocha -r \"../../scripts/import-expansions.js\" -r \"./tests/register-worker.js \" --timeout 15000 -r ts-node/register \"./src/**/*.spec.ts\"", "pretest-ci": "node ../../scripts/run-if-package-requested.js npm run webpack-build -- --no-stats --no-devtool", "test-ci": "node ../../scripts/run-if-package-requested.js npm test", "test-coverage": "nyc --no-clean --cwd ../.. --reporter=none npm run test", diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index 2eb2fa177..c4d706dd4 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -9,7 +9,6 @@ import type { import type { MongoshBus } from '@mongosh/types'; import path from 'path'; import { EventEmitter, once } from 'events'; -import Worker from 'web-worker'; import type { Caller } from './rpc'; import { createCaller, cancel, exposeAll } from './rpc'; import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './worker-runtime'; diff --git a/packages/node-runtime-worker-thread/tests/register-worker.js b/packages/node-runtime-worker-thread/tests/register-worker.js new file mode 100644 index 000000000..30f83f327 --- /dev/null +++ b/packages/node-runtime-worker-thread/tests/register-worker.js @@ -0,0 +1 @@ +global.Worker = require('web-worker'); From e7447de9165f048a1734c0e9c7ad38948a0cefc5 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Thu, 29 Aug 2024 09:17:51 +0200 Subject: [PATCH 25/26] fix paths for windows --- .../__fixtures__/script-that-throws.js | 1 - packages/node-runtime-worker-thread/src/index.ts | 7 +++---- .../node-runtime-worker-thread/src/worker-runtime.spec.ts | 3 ++- 3 files changed, 5 insertions(+), 6 deletions(-) delete mode 100644 packages/node-runtime-worker-thread/__fixtures__/script-that-throws.js diff --git a/packages/node-runtime-worker-thread/__fixtures__/script-that-throws.js b/packages/node-runtime-worker-thread/__fixtures__/script-that-throws.js deleted file mode 100644 index fbe4be2c2..000000000 --- a/packages/node-runtime-worker-thread/__fixtures__/script-that-throws.js +++ /dev/null @@ -1 +0,0 @@ -throw new Error("Nope, I'm not starting"); diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index c4d706dd4..5c417977c 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -9,6 +9,7 @@ import type { import type { MongoshBus } from '@mongosh/types'; import path from 'path'; import { EventEmitter, once } from 'events'; +import { pathToFileURL } from 'url'; import type { Caller } from './rpc'; import { createCaller, cancel, exposeAll } from './rpc'; import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './worker-runtime'; @@ -49,9 +50,7 @@ class WorkerRuntime implements Runtime { private workerProcessMongoshBus!: WorkerProcessMongoshBus; - private workerProcessPath = - process.env.TEST_WORKER_PATH ?? - path.resolve(__dirname, 'worker-runtime.js'); + private workerProcessPath = path.resolve(__dirname, 'worker-runtime.js'); constructor( uri: string, @@ -67,7 +66,7 @@ class WorkerRuntime implements Runtime { private async initWorker() { const workerProcess = new Worker( - this.workerProcessPath, + pathToFileURL(this.workerProcessPath).href, this.initOptions.workerOptions ); diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index e02a6b89e..41dc7a361 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -13,6 +13,7 @@ import type { WorkerRuntime } from './worker-runtime'; import type { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; import { interrupt } from 'interruptor'; import { dummyOptions } from './index.spec'; +import { pathToFileURL } from 'url'; chai.use(sinonChai); @@ -33,7 +34,7 @@ describe('worker-runtime', function () { let caller: Caller; beforeEach(async function () { - worker = new Worker(workerThreadModule); + worker = new Worker(pathToFileURL(workerThreadModule).href); await once(worker, 'message'); caller = createCaller( From ebdefc0af25b45ad7afa36b9936f01775909d1d3 Mon Sep 17 00:00:00 2001 From: Basit <1305718+mabaasit@users.noreply.github.com> Date: Mon, 2 Sep 2024 09:08:42 +0200 Subject: [PATCH 26/26] Update worker-runtime.ts --- packages/node-runtime-worker-thread/src/worker-runtime.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.ts b/packages/node-runtime-worker-thread/src/worker-runtime.ts index 20215f660..8176ca4fa 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.ts @@ -113,7 +113,6 @@ const workerRuntime: WorkerRuntime = { // TS2589: Type instantiation is excessively deep and possibly infinite. // I could not figure out why exactly that was the case, so 'as any' // will have to do for now. - // TODO: Do we still need error? provider = await CompassServiceProvider.connect( uri, deserializeConnectOptions(driverOptions),