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

node: extract node env initialization out of process initialization #980

Closed
wants to merge 1 commit into from

Conversation

petkaantonov
Copy link
Contributor

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?

// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Style: void* arg

Copy link
Member

Choose a reason for hiding this comment

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

If StartNodeInstance() is not used elsewhere, it should be static or inline (they amount to the same thing, non-external linkage.)

@bnoordhuis
Copy link
Member

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?

I'm not sure what comment that was but it sounds reasonable.

@vkurchatkin
Copy link
Contributor

Why do we need another class for this? Isn't Environment enough?

@petkaantonov
Copy link
Contributor Author

Addressed @bnoordhuis comments, I also added an internal CreateEnvironment overload since in the future we will need to know in env too whether it's the main thread or a worker one.

Why do we need another class for this? Isn't Environment enough?

Instances of this class will be the data argument passed to uv_create_thread, I think it's cleaner if we create Environment while already executing in the thread the environment will belong to.


// Main node instance uses uv_default_loop.
if (instance_data->is_worker())
uv_loop_delete(instance_data->event_loop());
Copy link
Member

Choose a reason for hiding this comment

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

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().

@bnoordhuis
Copy link
Member

LGTM with comments.

@petkaantonov
Copy link
Contributor Author

Addressed comments, rebased to v1.x and squashed

@petkaantonov
Copy link
Contributor Author

ping

exec_argc_(exec_argc),
exec_argv_(exec_argv),
use_debug_agent_flag_(use_debug_agent_flag) {
CHECK_NE(event_loop_, nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

Indent error.

@bnoordhuis
Copy link
Member

LGTM apart from a style nit. Land at will.

chrisdickinson pushed a commit that referenced this pull request Mar 5, 2015
@chrisdickinson
Copy link
Contributor

Merged in 4ae64b2. Thanks!

@rvagg
Copy link
Member

rvagg commented Mar 6, 2015

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?

@bnoordhuis
Copy link
Member

It's still completely internal, add-ons or embedders can't use it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants