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: make AtExit callback's per Environment #9163

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Oct 18, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change

This commit attempts to address one of the TODOs in
#4641 regarding making the AtExit
callback's per environment, instead of the current global.

bnoordhuis provided a few options for solving this and one was to use
a thread-local which is what this commit attempts to do..

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 18, 2016
@danbev
Copy link
Contributor Author

danbev commented Oct 18, 2016

src/node.cc Outdated
}


thread_local Environment* thread_local_env;
Copy link
Member

Choose a reason for hiding this comment

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

  • Should this be static?
  • I think this doesn’t need an explicit initializer, but would you mind adding = nullptr just for clarity?

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave out the = nullptr, it looks unidiomatic. It should be static though, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax @bnoordhuis I'd like to make sure I understand the reason for making this static. My understanding is that a thread_local object is allocated when the thread begins and deallocated when the thread ends, so static in this case does not refer to storage but to linkage. And without the explicitstatic, the linkage would be external by default. Does that sound correct?

Copy link
Member

Choose a reason for hiding this comment

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

Correct. static in this context is about linkage, not lifetime. Without static, the variable is visible to other compilation units.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis Thanks for clarifying that, I appreciate it.

src/env.h Outdated
AtExitCallback* next_;
void (*cb_)(void* arg);
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.

Just a suggestion, but you could make AtExitCallback a forward declaration in the node namespace here and only define it right before AtExit/RunAtExit. That would make sense insofar as the exact layout isn’t part of the API here, and having the definition of the struct close to its actual usage might be a tad more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good, I'll change that.

src/node.cc Outdated
delete p;
p = q;
}
env->RunAtExit();
Copy link
Member

Choose a reason for hiding this comment

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

CHECK_NE(env, nullptr);?

Copy link
Member

Choose a reason for hiding this comment

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

A CHECK won't hurt but OTOH, if it's a nullptr, the caller is going to find out soon enough. :-)

src/env.h Outdated
@@ -503,6 +503,9 @@ class Environment {

inline v8::Local<v8::Object> NewInternalFieldObject();

inline void AtExit(void (*cb)(void* arg), void* arg);
inline void RunAtExit();
Copy link
Member

Choose a reason for hiding this comment

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

RunAtExit was a confusing name when I first read it… I don’t have anything better in mind rn, but if you do, feel free to add that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about RunAtExitCallbacks?

src/node.h Outdated
@@ -209,7 +209,8 @@ NODE_EXTERN void FreeEnvironment(Environment* env);

NODE_EXTERN void EmitBeforeExit(Environment* env);
NODE_EXTERN int EmitExit(Environment* env);
NODE_EXTERN void RunAtExit(Environment* env);
NODE_DEPRECATED("Use Environment::RunAtExit()",
NODE_EXTERN void RunAtExit(Environment* env));
Copy link
Member

Choose a reason for hiding this comment

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

Oh, one more thing: env.h is not a public header, so if anyone actually wants to use RunAtExit in an addon, deprecating this puts them in quite the difficult position. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that does not sound good, I'll revert that :)

@addaleax
Copy link
Member

OS X CI failure: error: thread-local storage is not supported for the current target 😞 (output)

Maybe @bnoordhuis has a bit more context about who actually uses multiple Environments and in what way, I’ve always been wondering about that a bit.

src/env-inl.h Outdated
p->arg_ = arg;
p->next_ = at_exit_functions_;
at_exit_functions_ = p;
}
Copy link
Member

Choose a reason for hiding this comment

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

These don't really need to be inlined. I'd move it to src/env.cc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I'll move them.

src/node.cc Outdated
delete p;
p = q;
}
env->RunAtExit();
Copy link
Member

Choose a reason for hiding this comment

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

A CHECK won't hurt but OTOH, if it's a nullptr, the caller is going to find out soon enough. :-)

src/node.cc Outdated
}


thread_local Environment* thread_local_env;
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave out the = nullptr, it looks unidiomatic. It should be static though, yes.

@bnoordhuis
Copy link
Member

OS X CI failure: error: thread-local storage is not supported for the current target

Yeah, I was kind of expecting that. If we had still supported VS 2013, that would have been a problem, too. @danbev You can use uv_key_t instead.

Maybe @bnoordhuis has a bit more context about who actually uses multiple Environments and in what way, I’ve always been wondering about that a bit.

Embedders like electron and plask do. Electron for example has (or had) a context per tab.

@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 18, 2016

Forgot to mention, the OS X issue could be fixed by linking to libc++ instead of libstdc++ but that has some wider ranging implications and would make the pull request unsuitable for back-porting to v4.x and v6.x.

EDIT: Scratch that, we already seem to be doing that in master? I see a -stdlib=libc++ in the arguments.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@danbev
Copy link
Contributor Author

danbev commented Oct 19, 2016

@danbev You can use uv_key_t instead

I was not aware of uv_key_t, I'll take a look and try that out.

@danbev
Copy link
Contributor Author

danbev commented Oct 19, 2016

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Looking at this again, it doesn't seem quite correct. The thread_local is set once but of course you can more than one Environment per isolate.

It's currently mostly academic because StartNodeInstance() is only called from Start() but still.

src/env-inl.h Outdated
@@ -443,6 +443,7 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
return m_obj.ToLocalChecked();
}


Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary whitespace change.

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'll remove this.

src/env.h Outdated
@@ -578,6 +581,9 @@ class Environment {

char* http_parser_buffer_;

struct AtExitCallback;
AtExitCallback* at_exit_functions_ = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

For the record, I'd be okay with switching this a std::list<...>. Could be a separate commit or pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, let me do that in a separate PR.

@danbev
Copy link
Contributor Author

danbev commented Oct 20, 2016

Looking at this again, it doesn't seem quite correct. The thread_local is set once but of course you can more than one Environment per isolate.

It's currently mostly academic because StartNodeInstance() is only called from Start() but still.

I'll take another look into this

@danbev
Copy link
Contributor Author

danbev commented Oct 25, 2016

Rebased with upstream master.

Running CI again to see how much damage I've cause when updating node.gyp:
https://ci.nodejs.org/job/node-test-pull-request/4663/

This does not address @bnoordhuis point that Start can be called multiple times. I'll take a look at this next.

@danbev danbev force-pushed the at-exit-per-environment branch 6 times, most recently from c840ef1 to efac3b7 Compare November 3, 2016 07:06
@danbev
Copy link
Contributor Author

danbev commented Nov 3, 2016

Looking at this again, it doesn't seem quite correct. The thread_local is set once but of course you can more than one Environment per isolate.

Sorry about the delay on this. I've tried to address the issue of having multiple Environments per Isolate for the use case when Node is being embedded (see 45248a4d5f1794174a44a56186c5d9266ff75087 for details).

I'm trying to understand the other case when node::Start would be called multiple times. I've started by trying to create a test that calls node::Start multiple times it but failed quite early. The error reported is:

$ make cctest GTEST_FILTER=NodeTest.*

Note: Google Test filter = NodeTest.*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from NodeTest
[ RUN      ] NodeTest.StartMultipleTimes


#
# Fatal error in ../deps/v8/src/v8.cc, line 94
# Check failed: !platform_.
#

==== C stack trace ===============================

    0   cctest                              0x0000000100325b4e v8::base::debug::StackTrace::StackTrace() + 30
    1   cctest                              0x0000000100325b85 v8::base::debug::StackTrace::StackTrace() + 21
    2   cctest                              0x000000010031e914 V8_Fatal + 452
    3   cctest                              0x00000001010c996d v8::internal::V8::InitializePlatform(v8::Platform*) + 77
    4   cctest                              0x000000010035e8e5 v8::V8::InitializePlatform(v8::Platform*) + 21
    5   cctest                              0x0000000100033b20 _ZN4node3$_010InitializeEi + 48
    6   cctest                              0x000000010003371e node::Start(int, char**) + 158
    7   cctest                              0x00000001000cee4f startNode(void*) + 31
    8   libsystem_pthread.dylib             0x00007fff8423a99d _pthread_body + 131
    9   libsystem_pthread.dylib             0x00007fff8423a91a _pthread_body + 0
    10  libsystem_pthread.dylib             0x00007fff84238351 thread_start + 13
make: *** [cctest] Illegal instruction: 4

I know @bnoordhuis said that it was mostly academic, and I'm wondering if this is worth pursuing?
I'd be happy to continue but feel I might need some help/guidance of the best way to move forward.

Also, adding a cctest that used Node core bits was interesting and took a while to get it to pass on all platforms. Would it make sense to clean up test: enable gtest that use node core code and add this separately or is there any policy to avoid cctest in favour of other tests?

@danbev danbev force-pushed the at-exit-per-environment branch 3 times, most recently from 7859e8a to 0c2e578 Compare November 11, 2016 07:10
@bnoordhuis
Copy link
Member

Can you squash into logical commits? I understand you made some additional changes but the history is kind of hard to follow.

@danbev
Copy link
Contributor Author

danbev commented Nov 11, 2016

@bnoordhuis Sorry about that, most of them came about as I was trying to figure out how to get the test passing on the various platforms (after adding a test to cctest).

gibfahn pushed a commit to gibfahn/node that referenced this pull request Jun 17, 2017
This commit tries to make it simpler to add unit tests (cctest) for
code that needs to test node core funtionality but that might not be
appropriate as an addon or a JavaScript test. An example of this could
be adding functionality targeted for situations when Node itself is
embedded.

Currently it was not as easy, or efficient, as one would have hoped to
add such tests. The object output directories vary for different
operating systems which we need to link to so that we don't have an
additional compilation step.

PR-URL: nodejs#11956
Backport-PR-URL: nodejs#12948
Ref: nodejs#9163
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
This commit tries to make it simpler to add unit tests (cctest) for
code that needs to test node core funtionality but that might not be
appropriate as an addon or a JavaScript test. An example of this could
be adding functionality targeted for situations when Node itself is
embedded.

Currently it was not as easy, or efficient, as one would have hoped to
add such tests. The object output directories vary for different
operating systems which we need to link to so that we don't have an
additional compilation step.

PR-URL: #11956
Backport-PR-URL: #12948
Ref: #9163
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jul 18, 2017
this is a re-base of the gyp part of
6a09a69
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: nodejs#11956
Original-Ref: nodejs#9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#12450
Reviewed-By: João Reis <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
This change was suggested by bnoordhuis in the following comment:
nodejs/node#9163 (comment)

Not including any tests as this is covered by test/addons/at-exit.

PR-URL: nodejs/node#12255
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Aug 23, 2017
this is a re-base of the gyp part of
6a09a69
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: nodejs#11956
Original-Ref: nodejs#9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#12450
Reviewed-By: João Reis <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
this is a re-base of the gyp part of
6a09a69
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: nodejs/node#11956
Original-Ref: nodejs/node#9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#12450
Reviewed-By: João Reis <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
this is a re-base of the gyp part of
6a09a69
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: nodejs/node#11956
Original-Ref: nodejs/node#9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#12450
Reviewed-By: João Reis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
this is a re-base of the gyp part of
6a09a69
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: #11956
Original-Ref: #9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: #12450
Reviewed-By: João Reis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
this is a re-base of the gyp part of
6a09a69
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: #11956
Original-Ref: #9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: #12450
Reviewed-By: João Reis <[email protected]>
@MylesBorins
Copy link
Contributor

For right now @nodejs/lts decided to not land this as we didn't see a specific reason to backport. If this may be mistaken please let us know

@gibfahn
Copy link
Member

gibfahn commented Jan 17, 2018

Looks like this causes an issue with electron, see electron/electron#11299 (comment).

I don't know enough about this PR to be able to comment on whether the solution proposed in that thread is a good idea or not, but maybe @danbev @addaleax or @bnoordhuis might be able to offer some advice there.

@danbev
Copy link
Contributor Author

danbev commented Jan 18, 2018

@gibfahn I'll take a closer look at this (hopefully later today or tomorrow).

@danbev
Copy link
Contributor Author

danbev commented Jan 22, 2018

I've started to look into this, but won't be working for the next few (sick kid), but I'll revisit as soon as I'm back.

danbev added a commit to danbev/node that referenced this pull request Feb 13, 2018
This commit set the Environment as a thread local when CreateEnvironment
is called which is currently not being done. This would lead to a
segment fault if later node::AtExit is called without specifying the
environment parameter. This specific issue was reported by Electron.

If I recall correctly, back when this was implemented the motivation was
that if embedders have multiple environments per isolate they should be
using the AtExit functions that take an environment. This is not the
case with Electron which only create a single environment (as far as I
know), and if a native module calls AtExit this would lead to the
segment fault.

I was able to reproduce Electron issue and the provided test simulates
it. I was also able to use this patch and verify that it works for the
Electron issue as well.

Refs: nodejs#9163
Refs: electron/electron#11299
danbev added a commit to danbev/node that referenced this pull request Feb 16, 2018
This commit set the Environment as a thread local when CreateEnvironment
is called which is currently not being done. This would lead to a
segment fault if later node::AtExit is called without specifying the
environment parameter. This specific issue was reported by Electron.

If I recall correctly, back when this was implemented the motivation was
that if embedders have multiple environments per isolate they should be
using the AtExit functions that take an environment. This is not the
case with Electron which only create a single environment (as far as I
know), and if a native module calls AtExit this would lead to the
segment fault.

I was able to reproduce Electron issue and the provided test simulates
it. I was also able to use this patch and verify that it works for the
Electron issue as well.

PR-URL: nodejs#18573
Refs: nodejs#9163
Refs: electron/electron#11299
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
This commit set the Environment as a thread local when CreateEnvironment
is called which is currently not being done. This would lead to a
segment fault if later node::AtExit is called without specifying the
environment parameter. This specific issue was reported by Electron.

If I recall correctly, back when this was implemented the motivation was
that if embedders have multiple environments per isolate they should be
using the AtExit functions that take an environment. This is not the
case with Electron which only create a single environment (as far as I
know), and if a native module calls AtExit this would lead to the
segment fault.

I was able to reproduce Electron issue and the provided test simulates
it. I was also able to use this patch and verify that it works for the
Electron issue as well.

PR-URL: nodejs#18573
Refs: nodejs#9163
Refs: electron/electron#11299
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This commit set the Environment as a thread local when CreateEnvironment
is called which is currently not being done. This would lead to a
segment fault if later node::AtExit is called without specifying the
environment parameter. This specific issue was reported by Electron.

If I recall correctly, back when this was implemented the motivation was
that if embedders have multiple environments per isolate they should be
using the AtExit functions that take an environment. This is not the
case with Electron which only create a single environment (as far as I
know), and if a native module calls AtExit this would lead to the
segment fault.

I was able to reproduce Electron issue and the provided test simulates
it. I was also able to use this patch and verify that it works for the
Electron issue as well.

PR-URL: nodejs#18573
Refs: nodejs#9163
Refs: electron/electron#11299
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
rvagg pushed a commit to nodejs/node-gyp that referenced this pull request Aug 9, 2018
this is a re-base of the gyp part of
6a09a69ec9d36b705e9bde2ac1a193566a702d96
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: nodejs/node#11956
Original-Ref: nodejs/node#9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#12450
Reviewed-By: João Reis <[email protected]>
rvagg pushed a commit to nodejs/node-gyp that referenced this pull request Aug 9, 2018
this is a re-base of the gyp part of
6a09a69ec9d36b705e9bde2ac1a193566a702d96
after bumping GYP version to
https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161

Original-PR-URL: nodejs/node#11956
Original-Ref: nodejs/node#9163
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#12450
Reviewed-By: João Reis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants