src: moving dispatch_debug_messages_async to Environment #7358
Conversation
nodejs-github-bot
added
the
c++
|
The intent of the TODO is to make the async handle a non-static Environment member, with the larger goal of being able to start a debugger per context. Making the handle a static member doesn't gain us much, it's really just moving code around. |
|
@bnoordhuis Ah, that makes sense. I was not entirely sure when reading the TODO comment but thought it would become clear with a PR which it has. Let me take another stab at this. |
This commit attempts to fix one of the items in nodejs#4641, which was to move dispatch_debug_messages_async to the Environment class.
danbev
force-pushed
the
move-dispatch-debug-message-async-to-env
branch
from
56a9199
to
98b8689
Compare
|
@bnoordhuis I've made another attempt at this. When you get a chance, please take a look and let me know if this is what you had in mind. Thanks |
| 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); |
There was a problem hiding this comment.
Can you use static_cast here?
|
@bnoordhuis thanks for the review! I've updated the PR with commits to address your comments. |
| uv_unref(reinterpret_cast<uv_handle_t*>( | ||
| env.dispatch_debug_messages_async())); | ||
|
|
||
| if (!use_debug_agent) { |
There was a problem hiding this comment.
You might as well use instance_data->use_debug_agent() now that it's been moved to StartNodeInstance().
There was a problem hiding this comment.
I was not aware of that method, I'll update this.
|
LGTM with comments. |
| env.dispatch_debug_messages_async())); | ||
|
|
||
| if (!instance_data->use_debug_agent()) { | ||
| RegisterDebugSignalHandler(env.dispatch_debug_messages_async()); |
There was a problem hiding this comment.
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.
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?
|
Closing this with the reason given by @bnoordhuis in #7358 (comment) |
Checklist
make -j4 test(UNIX) orvcbuild test nosign(Windows) passesAffected core subsystem(s)
src
Description of change
This commit attempts to fix one of the items in
#4641, which was to move
dispatch_debug_messages_async to the Environment class.