module: warn of potential for deadlock with hooks worker

doc/api/module.md

@@ -111,6 +111,17 @@ changes:
 Register a module that exports [hooks][] that customize Node.js module
 resolution and loading behavior. See [Customization hooks][].
 
+> **Warning** When setting up a `MessageChannel` to communicate with hooks,
+> beware that this can lead to a deadlock. For example, you have 2 modules,
+> A and B. "A" is registered first and sets up a message channel, which it uses
+> in its `resolve` hook. After "A" is registered, "B" is registered. Resolving
+> "B"'s specifier will go through "A"'s `resolve` hook, which will try to
+> communicate with a locked thread that is busy trying to register "B"'s hooks.
+> Since registering "B" depends on resolving "B"'s specifier, and resolving
+> "B"'s specifier is blocked by "A"'s communication request that is itself
+> blocked by the pending registration that started the chain, the application
+> becomes deadlocked.
+
 ### `module.syncBuiltinESMExports()`
 
 <!-- YAML

test/fixtures/es-module-loaders/inter-dependent-hooks/a/hooks.mjs

@@ -0,0 +1,23 @@
+let port;
+
+export function initialize(data) {
+  ({ port } = data);
+}
+
+export function resolve(specifier, context, nextResolve) {
+  const id = ''+Math.random();
+  port.postMessage({
+    context,
+    id,
+    specifier,
+  });
+
+  return new Promise((resolve) => {
+    port.on('message', (message) => {
+      if (message.id !== id) return;
+
+      port.off('message', onMessage);
+      resolve({ url: message.specifier });
+    });
+  });
+}

test/fixtures/es-module-loaders/inter-dependent-hooks/a/register.mjs

@@ -0,0 +1,15 @@
+import { register } from 'node:module';
+import { MessageChannel } from 'node:worker_threads';
+
+const { port1, port2 } = new MessageChannel();
+
+port1.unref();
+port2.unref();
+
+register((new URL('./hooks.mjs', import.meta.url)).href, {
+  data: {
+    port: port2,
+  },
+  parentURL: import.meta.url,
+  transferList: [port2],
+});

test/fixtures/es-module-loaders/inter-dependent-hooks/b/hooks.mjs

@@ -0,0 +1,9 @@
+// This file will not get loaded because of a deadlock trying to resolve it.
+
+export function resolve(specifier, context, nextResolve) {
+  return nextResolve(specifier, context, nextResolve);
+}
+
+export function load(url, context, nextLoad) {
+  return nextLoad(url, context, nextLoad);
+}

test/fixtures/es-module-loaders/inter-dependent-hooks/b/register.mjs

@@ -0,0 +1,9 @@
+import { register } from 'node:module';
+import { MessageChannel } from 'node:worker_threads';
+
+const { port1, port2 } = new MessageChannel();
+
+port1.unref();
+port2.unref();
+
+register((new URL('./hooks.mjs', import.meta.url)).href);

test/known_issues/test-hooks-thread-deadlock.js

@@ -0,0 +1,43 @@
+const { mustNotCall } = require('../common/index.js');
+const fixtures = require('../common/fixtures');
+const assert = require('node:assert');
+const { spawn } = require('node:child_process');
+const { execPath } = require('node:process');
+const { describe, it } = require('node:test');
+const { pathToFileURL } = require('node:url');
+
+describe('hooks deadlock', { concurrency: true }, () => {
+  it('will deadlock when a/hooks…resolve tries to contact the main thread during b/register', async () => {
+    let stderr = '';
+    let stdout = '';
+    // ! Do NOT use spawnSync here: it will deadlock.
+    const child = spawn(execPath, [
+      '--import', fixtures.fileURL('es-module-loaders/inter-dependent-hooks/a/register.mjs')),
+      '--import', fixtures.fileURL('es-module-loaders/inter-dependent-hooks/b/register.mjs')),
+      '--input-type=module',
+      '--eval',
+      'import.meta.url;console.log("done")',
+    ]);
+    child.stderr.setEncoding('utf8');
+    child.stderr.on('data', (data) => { stderr += data; });
+    child.stdout.setEncoding('utf8');
+    child.stdout.on('data', (data) => { stdout += data; });
+
+    child.on('close', () => mustNotCall('Deadlock should prevent closing'));
+
+    return new Promise((res, rej) => {
+      setTimeout(() => {
+        try {
+          assert.strictEqual(stderr, '');
+          assert.strictEqual(child.exitCode, null); // It hasn't ended
+          assert.strictEqual(child.signalCode, null); // It hasn't ended
+
+          assert.strictEqual(child.kill('SIGKILL'), true); // Deadlocked process must be forcibily killed.
+          res('deadlocked process terminated');
+        } catch (e) {
+          rej(e);
+        }
+      }, 5_000); // This should have finished well before 5 second.
+    });
+  });
+});