Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix Worker termination in inspector.waitForDebugger #52527

Merged
merged 9 commits into from
May 13, 2024
7 changes: 7 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1092,6 +1092,13 @@ void Environment::InitializeLibuv() {
void Environment::ExitEnv(StopFlags::Flags flags) {
// Should not access non-thread-safe methods here.
set_stopping(true);

#if HAVE_INSPECTOR
if (inspector_agent_) {
inspector_agent_->StopIfWaitingForConnect();
}
#endif

if ((flags & StopFlags::kDoNotTerminateIsolate) == 0)
isolate_->TerminateExecution();
SetImmediateThreadsafe([](Environment* env) {
Expand Down
11 changes: 10 additions & 1 deletion src/inspector/main_thread_interface.cc
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,20 @@ bool MainThreadInterface::WaitForFrontendEvent() {
dispatching_messages_ = false;
if (dispatching_message_queue_.empty()) {
Mutex::ScopedLock scoped_lock(requests_lock_);
while (requests_.empty()) incoming_message_cond_.Wait(scoped_lock);
while (!stop_waiting_for_frontend_event_requested_ && requests_.empty()) {
incoming_message_cond_.Wait(scoped_lock);
}
stop_waiting_for_frontend_event_requested_ = false;
}
return true;
}

void MainThreadInterface::StopWaitingForFrontendEvent() {
daeyeon marked this conversation as resolved.
Show resolved Hide resolved
Mutex::ScopedLock scoped_lock(requests_lock_);
stop_waiting_for_frontend_event_requested_ = true;
incoming_message_cond_.Broadcast(scoped_lock);
}

void MainThreadInterface::DispatchMessages() {
if (dispatching_messages_)
return;
Expand Down
5 changes: 5 additions & 0 deletions src/inspector/main_thread_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class MainThreadInterface :
void DispatchMessages();
void Post(std::unique_ptr<Request> request);
bool WaitForFrontendEvent();
void StopWaitingForFrontendEvent();
std::shared_ptr<MainThreadHandle> GetHandle();
Agent* inspector_agent() {
return agent_;
Expand All @@ -94,6 +95,10 @@ class MainThreadInterface :
// when we reenter the DispatchMessages function.
MessageQueue dispatching_message_queue_;
bool dispatching_messages_ = false;
// This flag indicates an internal request to exit the loop in
// WaitForFrontendEvent(). It's set to true by calling
// StopWaitingForFrontendEvent().
bool stop_waiting_for_frontend_event_requested_ = false;
daeyeon marked this conversation as resolved.
Show resolved Hide resolved
ConditionVariable incoming_message_cond_;
// Used from any thread
Agent* const agent_;
Expand Down
21 changes: 21 additions & 0 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,20 @@ class NodeInspectorClient : public V8InspectorClient {
}
}

void StopIfWaitingForFrontendEvent() {
if (!waiting_for_frontend_) {
return;
}
waiting_for_frontend_ = false;
daeyeon marked this conversation as resolved.
Show resolved Hide resolved
for (const auto& id_channel : channels_) {
id_channel.second->unsetWaitingForDebugger();
}

if (interface_) {
interface_->StopWaitingForFrontendEvent();
}
}

int connectFrontend(std::unique_ptr<InspectorSessionDelegate> delegate,
bool prevent_shutdown) {
int session_id = next_session_id_++;
Expand Down Expand Up @@ -1017,6 +1031,13 @@ void Agent::WaitForConnect() {
client_->waitForFrontend();
}

void Agent::StopIfWaitingForConnect() {
if (client_ == nullptr) {
return;
}
client_->StopIfWaitingForFrontendEvent();
}

std::shared_ptr<WorkerManager> Agent::GetWorkerManager() {
THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_,
permission::PermissionScope::kInspector,
Expand Down
2 changes: 2 additions & 0 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ class Agent {

// Blocks till frontend connects and sends "runIfWaitingForDebugger"
void WaitForConnect();
void StopIfWaitingForConnect();

// Blocks till all the sessions with "WaitForDisconnectOnShutdown" disconnect
void WaitForDisconnect();
void ReportUncaughtException(v8::Local<v8::Value> error,
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';

// 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));
Copy link
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.

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
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
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());
await worker.terminate();
} else {
inspector.open();
inspector.waitForDebugger();
}
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
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
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)

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