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: moving dispatch_debug_messages_async to Environment #7358

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ inline uv_idle_t* Environment::immediate_idle_handle() {
return &immediate_idle_handle_;
}

inline uv_async_t* Environment::dispatch_debug_messages_async() {
return &dispatch_debug_messages_async_;
}

inline void Environment::RegisterHandleCleanup(uv_handle_t* handle,
HandleCleanupCb cb,
void *arg) {
Expand Down
2 changes: 2 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ class Environment {
static inline Environment* from_immediate_check_handle(uv_check_t* handle);
inline uv_check_t* immediate_check_handle();
inline uv_idle_t* immediate_idle_handle();
inline uv_async_t* dispatch_debug_messages_async();

// Register clean-up cb to be called on environment destruction.
inline void RegisterHandleCleanup(uv_handle_t* handle,
Expand Down Expand Up @@ -586,6 +587,7 @@ class Environment {
uv_idle_t immediate_idle_handle_;
uv_prepare_t idle_prepare_handle_;
uv_check_t idle_check_handle_;
uv_async_t dispatch_debug_messages_async_;
AsyncHooks async_hooks_;
DomainFlag domain_flag_;
TickInfo tick_info_;
Expand Down
47 changes: 26 additions & 21 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ bool config_preserve_symlinks = false;
// process-relative uptime base, initialized at start-up
static double prog_start_time;
static bool debugger_running;
static uv_async_t dispatch_debug_messages_async;

static Mutex node_isolate_mutex;
static v8::Isolate* node_isolate;
Expand Down Expand Up @@ -3643,8 +3642,7 @@ static void ParseArgs(int* argc,

// Called from V8 Debug Agent TCP thread.
static void DispatchMessagesDebugAgentCallback(Environment* env) {
// TODO(indutny): move async handle to environment
uv_async_send(&dispatch_debug_messages_async);
uv_async_send(env->dispatch_debug_messages_async());
}


Expand Down Expand Up @@ -3697,11 +3695,11 @@ static void EnableDebug(Environment* env) {


// Called from an arbitrary thread.
static void TryStartDebugger() {
static void TryStartDebugger(uv_async_t* dispatch_debug_messages_async) {
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
if (auto isolate = node_isolate) {
v8::Debug::DebugBreak(isolate);
uv_async_send(&dispatch_debug_messages_async);
uv_async_send(dispatch_debug_messages_async);
}
}

Expand Down Expand Up @@ -3768,16 +3766,20 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
}


inline void* DebugSignalThreadMain(void* unused) {
inline void* DebugSignalThreadMain(void* async_handler) {
uv_async_t* dispatch_debug_messages_async =
reinterpret_cast<uv_async_t*>(async_handler);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use static_cast here?

CHECK_NE(dispatch_debug_messages_async, nullptr);
for (;;) {
uv_sem_wait(&debug_semaphore);
TryStartDebugger();
TryStartDebugger(dispatch_debug_messages_async);
}
return nullptr;
}


static int RegisterDebugSignalHandler() {
static int RegisterDebugSignalHandler(
uv_async_t* dispatch_debug_messages_async) {
Copy link
Member

Choose a reason for hiding this comment

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

Tiny style nit: can you use four spaces for line continuations?

// Start a watchdog thread for calling v8::Debug::DebugBreak() because
// it's not safe to call directly from the signal handler, it can
// deadlock with the thread it interrupts.
Expand All @@ -3795,8 +3797,10 @@ static int RegisterDebugSignalHandler() {
sigfillset(&sigmask);
CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, &sigmask));
pthread_t thread;
const int err =
pthread_create(&thread, &attr, DebugSignalThreadMain, nullptr);
const int err = pthread_create(&thread,
&attr,
DebugSignalThreadMain,
dispatch_debug_messages_async);
CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, nullptr));
CHECK_EQ(0, pthread_attr_destroy(&attr));
if (err != 0) {
Expand Down Expand Up @@ -4057,13 +4061,6 @@ void Init(int* argc,
// Make inherited handles noninheritable.
uv_disable_stdio_inheritance();

// init async debug messages dispatching
// Main thread uses uv_default_loop
uv_async_init(uv_default_loop(),
&dispatch_debug_messages_async,
DispatchDebugMessagesAsyncCallback);
uv_unref(reinterpret_cast<uv_handle_t*>(&dispatch_debug_messages_async));

#if defined(NODE_V8_OPTIONS)
// Should come before the call to V8::SetFlagsFromCommandLine()
// so the user can disable a flag --foo at run-time by passing
Expand Down Expand Up @@ -4130,10 +4127,6 @@ void Init(int* argc,
const char no_typed_array_heap[] = "--typed_array_max_size_in_heap=0";
V8::SetFlagsFromString(no_typed_array_heap, sizeof(no_typed_array_heap) - 1);

if (!use_debug_agent) {
RegisterDebugSignalHandler();
}

// We should set node_is_initialized here instead of in node::Start,
// otherwise embedders using node::Init to initialize everything will not be
// able to set it and native modules will not load for them.
Expand Down Expand Up @@ -4270,7 +4263,19 @@ static void StartNodeInstance(void* arg) {
array_buffer_allocator.zero_fill_field());
Local<Context> context = Context::New(isolate);
Context::Scope context_scope(context);

Copy link
Member

Choose a reason for hiding this comment

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

Can you undo the blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, will do.

Environment env(&isolate_data, context);

uv_async_init(uv_default_loop(),
Copy link
Member

Choose a reason for hiding this comment

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

This should use env.event_loop().

env.dispatch_debug_messages_async(),
DispatchDebugMessagesAsyncCallback);
uv_unref(reinterpret_cast<uv_handle_t*>(
env.dispatch_debug_messages_async()));

if (!use_debug_agent) {
Copy link
Member

Choose a reason for hiding this comment

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

You might as well use instance_data->use_debug_agent() now that it's been moved to StartNodeInstance().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware of that method, I'll update this.

RegisterDebugSignalHandler(env.dispatch_debug_messages_async());
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just realized this is probably not going to work. It installs the signal handler (and starts the watchdog thread, initializes the debug semaphore, etc.) for every context. It's not academic: Electron creates multiple contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I just realized this is probably not going to work. It installs the signal handler (and starts the watchdog thread, initializes the debug semaphore, etc.) for every context. It's not academic: Electron creates multiple contexts.

No worries, I appreciate your help and learned for doing it. Just to clarify, is that Electron that you are referring to?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that Electron.

}

env.Start(instance_data->argc(),
instance_data->argv(),
instance_data->exec_argc(),
Expand Down