src: fix Worker termination in inspector.waitForDebugger

[daeyeon]

Fixes: #52467

When calling inspector.waitForDebugger() on a worker thread, it waits synchronously for debugger to connect using a conditional variable, incoming_message_cond_. In this state, the worker thread cannot be terminated by the main thread.

This patch adds a way to exit the wait state when node process exits.

Signed-off-by: Daeyeon Jeong [email protected]

[daeyeon]

/cc @nodejs/cpp-reviewers @nodejs/inspector @nodejs/workers

[benjamingr]

Nice analysis!

[eugeneo]

1. I suggest merging the test cases - start worker # 1, terminate it, start worker # 2, call process.exit
2. What would happen if the main thread runs to completion? Would the worker just stay hanging? I don't remember details, would the WebSocket server shut down when the main thread completes and there's no ongoing sessions?

I believe the broader question is if inspector.waitForDebugger even makes sense for workers. The way I remember target domain is you either specify autoattach so existing session would attach to newly created workers, or workers just start and sessions are attached whenever.

In the first case (autoattach) worker should only wait if there are connected sessions that asked to be attached to workers.
In the second case worker do not need to wait.

I apologize if I am not making sense, haven't worked on this in years...

[eugeneo]

(Feel free to dismiss my comments as I am not sure I am up to date with the current state of inspector)

[AriPerkkio]

I believe the broader question is if inspector.waitForDebugger even makes sense for workers. The way I remember target domain is you either specify autoattach so existing session would attach to newly created workers, or workers just start and sessions are attached whenever.

As far as I know, inspector.waitForDebugger is the only working way to (programatically) debug Workers. #26609 (comment)

[daeyeon]

I suggest merging the test cases

Applied.

What would happen if the main thread runs to completion?

If the main thread runs to completion (assuming it runs without process.exit() in this test), then the WS server doesn't shut down, leaving the worker hanging.

Users seem to be using inspector.waitForDebugger as a valid workaround for now. I did several tries, but autoattach doesn't seem to work.

[eugeneo]

Ack. Thank you for the explanations.

[legendecas]

Using timeout to wait until a worker is ready can be flaky on slow CI machines.

[AriPerkkio]

This could be more stable approach:

// Flags: --inspect=0
import * as common from '../common/index.mjs';
common.skipIfInspectorDisabled();

-import { isMainThread, Worker } from 'node:worker_threads';
+import { isMainThread, parentPort, Worker } from 'node:worker_threads';
import { fileURLToPath } from 'node:url';
import inspector from 'node:inspector';

// Ref: https://github.com/nodejs/node/issues/52467
// - worker.terminate() should terminate the worker and the pending
//   inspector.waitForDebugger().
if (isMainThread) {
  const worker = new Worker(fileURLToPath(import.meta.url));
- await new Promise((r) => setTimeout(r, 2_000));
+ await new Promise((r) => worker.on("message", r));

  worker.on('exit', common.mustCall());
  await worker.terminate();
} else {
  inspector.open();
+ parentPort.postMessage("inspector is open")
  inspector.waitForDebugger();
}

[daeyeon]

Updated using MessagePort. Thanks. PTAL.

[daeyeon]

Using MessagePort doesn't seem to be enough. I tried using common.platformTimeout() to adjust the timeout to give the worker more time.

[daeyeon]

Changed the test to .js to fix the failed CI test, node-test-linux-linked-withoutssl.

common.skipIfInspectorDisabled() skips the test when the V8 inspector is disabled using either --without-inspector or --without-ssl. Since import statements are hoisted, importing node:inspector threw an error before executing common.skipIfInspectorDisabled() on the CI test.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58453/

[nodejs-github-bot]

Landed in ac5f8f0