-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
Changes from 1 commit
98b8689
cc7279f
0870bcb
5b430fa
11cc99f
c8b9c3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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()); | ||
} | ||
|
||
|
||
|
@@ -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); | ||
} | ||
} | ||
|
||
|
@@ -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); | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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) { | ||
|
@@ -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 | ||
|
@@ -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. | ||
|
@@ -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); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you undo the blank line? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use |
||
env.dispatch_debug_messages_async(), | ||
DispatchDebugMessagesAsyncCallback); | ||
uv_unref(reinterpret_cast<uv_handle_t*>( | ||
env.dispatch_debug_messages_async())); | ||
|
||
if (!use_debug_agent) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might as well use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No worries, I appreciate your help and learned for doing it. Just to clarify, is that Electron that you are referring to? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
|
There was a problem hiding this comment.
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?