Skip to content

Commit

Permalink
esm: do not use 'beforeExit' on the main thread
Browse files Browse the repository at this point in the history
PR-URL: nodejs/node#47964
Backport-PR-URL: nodejs/node#50669
Fixes: nodejs/node#47929
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
  • Loading branch information
sercher committed Apr 25, 2024
1 parent 1ef0ca4 commit 721b5d9
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 17 deletions.
21 changes: 4 additions & 17 deletions graal-nodejs/lib/internal/modules/esm/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -520,14 +520,6 @@ class HooksProxy {
}
}

#beforeExitHandler = () => {
debug('beforeExit main thread', this.#lock, this.#numberOfPendingAsyncResponses);
if (this.#numberOfPendingAsyncResponses !== 0) {
// The worker still has some work to do, let's wait for it before terminating the process.
this.#worker.ref();
}
};

async makeAsyncRequest(method, ...args) {
this.#waitForWorker();

Expand All @@ -543,11 +535,9 @@ class HooksProxy {
// come AFTER the last task in the event loop has run its course and there would be nothing
// left keeping the thread alive (and once the main thread dies, the whole process stops).
// However we want to keep the process alive until the worker thread responds (or until the
// event loop of the worker thread is also empty). So we add the beforeExit handler whose
// mission is to lock the main thread until we hear back from the worker thread. The `if`
// condition is there so we only add the event handler once (if there are already pending
// async responses, the previous calls have added the event listener).
process.on('beforeExit', this.#beforeExitHandler);
// event loop of the worker thread is also empty), so we ref the worker until we get all the
// responses back.
this.#worker.ref();
}

let response;
Expand All @@ -556,16 +546,13 @@ class HooksProxy {
await AtomicsWaitAsync(this.#lock, WORKER_TO_MAIN_THREAD_NOTIFICATION, this.#workerNotificationLastId).value;
this.#workerNotificationLastId = AtomicsLoad(this.#lock, WORKER_TO_MAIN_THREAD_NOTIFICATION);

// In case the beforeExit handler was called during the await, we revert its actions.
this.#worker.unref();

response = receiveMessageOnPort(asyncCommChannel.port1);
} while (response == null);
debug('got async response from worker', { method, args }, this.#lock);

if (--this.#numberOfPendingAsyncResponses === 0) {
// We got all the responses from the worker, its job is done (until next time).
process.off('beforeExit', this.#beforeExitHandler);
this.#worker.unref();
}

const body = this.#unwrapMessage(response);
Expand Down
16 changes: 16 additions & 0 deletions graal-nodejs/test/es-module/test-esm-loader-hooks.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,20 @@ describe('Loader hooks', { concurrency: true }, () => {
assert.strictEqual(code, 42);
assert.strictEqual(signal, null);
});

it('should be fine to call `process.removeAllListeners("beforeExit")` from the main thread', async () => {
const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
'--no-warnings',
'--experimental-loader',
'data:text/javascript,export function load(a,b,c){return new Promise(d=>setTimeout(()=>d(c(a,b)),99))}',
'--input-type=module',
'--eval',
'setInterval(() => process.removeAllListeners("beforeExit"),1).unref();await import("data:text/javascript,")',
]);

assert.strictEqual(stderr, '');
assert.strictEqual(stdout, '');
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});
});

0 comments on commit 721b5d9

Please sign in to comment.