node: extract node env initialization out of process initialization #980
Conversation
petkaantonov
force-pushed
the
threadstuffs
branch
from
8a90f3f to
31313f1
Compare
| // Fetch a reference to the main isolate, so we have a reference to it | ||
| // Entry point for new node instances, also called directly for the main | ||
| // node instance. | ||
| void StartNodeInstance(void *arg) { |
There was a problem hiding this comment.
If StartNodeInstance() is not used elsewhere, it should be static or inline (they amount to the same thing, non-external linkage.)
I'm not sure what comment that was but it sounds reasonable. |
|
Why do we need another class for this? Isn't |
petkaantonov
force-pushed
the
threadstuffs
branch
from
31313f1 to
f234211
Compare
|
Addressed @bnoordhuis comments, I also added an internal
Instances of this class will be the data argument passed to |
|
|
||
| // Main node instance uses uv_default_loop. | ||
| if (instance_data->is_worker()) | ||
| uv_loop_delete(instance_data->event_loop()); |
There was a problem hiding this comment.
Looking at this again, I think it should be left to the caller to clean up the uv_loop_t. Calling uv_loop_delete() is not safe because we don't know if the loop was created with uv_loop_new() or uv_loop_init().
|
LGTM with comments. |
petkaantonov
force-pushed
the
threadstuffs
branch
from
e167887 to
411d78f
Compare
|
Addressed comments, rebased to v1.x and squashed |
|
ping |
| exec_argc_(exec_argc), | ||
| exec_argv_(exec_argv), | ||
| use_debug_agent_flag_(use_debug_agent_flag) { | ||
| CHECK_NE(event_loop_, nullptr); |
|
LGTM apart from a style nit. Land at will. |
petkaantonov
force-pushed
the
threadstuffs
branch
from
411d78f to
dfe0b40
Compare
PR-URL: #980 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Merged in 4ae64b2. Thanks! |
|
this feels like something that should go into Notable items in the 1.5.0 release notes but I don't really know how best to characterise it for casual readers. Does somebody want to take a stab at a description in #1060 for me before this goes out? |
|
It's still completely internal, add-ons or embedders can't use it yet. |
This will enable implementing a worker class that is backed by a thread instead of a separate process.
/cc @bnoordhuis
P.S. I saw your todo comments about using uv_default_loop, I was thinking why not have the main thread use the default loop and workers use their own loops?