Skip to content
Merged
Prev Previous commit
Next Next commit
separate tc for clarify on patch expectations
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
  • Loading branch information
daeyeon committed Apr 15, 2024
commit 9c210e91ee2d915313ecdc2802b8a2839c24f598
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,14 @@ 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, 1_000));
await new Promise((r) => setTimeout(r, 2_000));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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();
}

Copy link
Copy Markdown
Member Author

@daeyeon daeyeon Apr 16, 2024

Choose a reason for hiding this comment

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

Updated using MessagePort. Thanks. PTAL.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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


worker.on('exit', common.mustCall());

const timeout = setTimeout(() => {
process.exit();
}, 2_000).unref();

await worker.terminate();

clearTimeout(timeout);
} else {
inspector.open();
inspector.waitForDebugger();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Flags: --inspect=0
import * as common from '../common/index.mjs';
common.skipIfInspectorDisabled();

import { isMainThread, Worker } from 'node:worker_threads';
import { fileURLToPath } from 'node:url';
import inspector from 'node:inspector';
import assert from 'node:assert';

// Ref: https://github.com/nodejs/node/issues/52467
// - process.exit() should kill the main thread's process.
if (isMainThread) {
new Worker(fileURLToPath(import.meta.url));
await new Promise((r) => setTimeout(r, 2_000));

process.on('exit', (status) => assert.strictEqual(status, 0));
process.exit();
Copy link
Copy Markdown
Contributor

@eugeneo eugeneo Apr 15, 2024

Choose a reason for hiding this comment

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

  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...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ack. Thank you for the explanations.

} else {
inspector.open();
inspector.waitForDebugger();
}