src: make AtExit callback's per Environment #9163

danbev · 2016-10-18T14:59:23Z

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

danbev · 2016-10-18T15:09:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4560/

addaleax · 2016-10-18T15:06:54Z

src/node.cc

 }


+thread_local Environment* thread_local_env;


Should this be static?

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

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

@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?

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

@bnoordhuis Thanks for clarifying that, I appreciate it.

addaleax · 2016-10-18T15:08:52Z

src/env.h

+    AtExitCallback* next_;
+    void (*cb_)(void* arg);
+    void* arg_;
+  };


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.

That sounds good, I'll change that.

addaleax · 2016-10-18T15:12:27Z

src/node.cc

-    delete p;
-    p = q;
-  }
+  env->RunAtExit();


CHECK_NE(env, nullptr);?

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

addaleax · 2016-10-18T15:19:35Z

src/env.h

@@ -503,6 +503,9 @@ class Environment {

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

+  inline void AtExit(void (*cb)(void* arg), void* arg);
+  inline void RunAtExit();


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.

How about RunAtExitCallbacks?

addaleax · 2016-10-18T15:23:18Z

src/node.h

@@ -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));


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

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

addaleax · 2016-10-18T15:26:29Z

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.

bnoordhuis · 2016-10-18T15:49:37Z

src/env-inl.h

+  p->arg_ = arg;
+  p->next_ = at_exit_functions_;
+  at_exit_functions_ = p;
+}


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

Great, I'll move them.

bnoordhuis · 2016-10-18T15:51:35Z

src/node.cc

-    delete p;
-    p = q;
-  }
+  env->RunAtExit();


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

bnoordhuis · 2016-10-18T15:56:23Z

src/node.cc

 }


+thread_local Environment* thread_local_env;


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

bnoordhuis · 2016-10-18T16:00:49Z

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 · 2016-10-18T16:03:20Z

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.

danbev · 2016-10-19T08:21:40Z

@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 · 2016-10-19T12:06:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4574/

bnoordhuis

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.

bnoordhuis · 2016-10-20T11:39:17Z

src/env-inl.h

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

+


Unnecessary whitespace change.

I'll remove this.

bnoordhuis · 2016-10-20T11:45:08Z

src/env.h

@@ -578,6 +581,9 @@ class Environment {

  char* http_parser_buffer_;

+  struct AtExitCallback;
+  AtExitCallback* at_exit_functions_ = nullptr;


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

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

danbev · 2016-10-20T12:24:10Z

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 · 2016-10-25T15:07:09Z

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 · 2016-11-03T09:07:14Z

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?

bnoordhuis · 2016-11-11T09:13:04Z

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

danbev · 2016-11-11T09:33:23Z

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

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]>

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]>

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]>

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]>

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]>

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]>

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 · 2017-09-19T21:11:10Z

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 · 2018-01-17T23:22:17Z

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 · 2018-01-18T10:05:52Z

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

danbev · 2018-01-22T07:44:47Z

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.

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

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]>

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]>

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

danbev mentioned this pull request Oct 18, 2016

src: make Environment optional for RunAtExit #9097

Closed

2 tasks

addaleax reviewed Oct 18, 2016

View reviewed changes

bnoordhuis requested changes Oct 18, 2016

View reviewed changes

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

bnoordhuis requested changes Oct 20, 2016

View reviewed changes

danbev force-pushed the at-exit-per-environment branch from 27de753 to a7c59da Compare October 25, 2016 14:52

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

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

danbev force-pushed the at-exit-per-environment branch from 0c2e578 to e63668d Compare November 11, 2016 09:31

MylesBorins mentioned this pull request Aug 16, 2017

(v6.x backport) src: remove GTEST_DONT_DEFINE_ASSERT_EQ in util.h #14836

Closed

2 tasks

MylesBorins removed the lts-watch-v6.x label Sep 19, 2017

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

gibfahn mentioned this pull request Jan 17, 2018

Electron 1.8.x crashes loading native module with node::AtExit() electron/electron#11299

Closed

danbev mentioned this pull request Feb 5, 2018

src: set thread local env in CreateEnvironment #18573

Closed

3 tasks

ewanharris mentioned this pull request Apr 21, 2021

feat: support node 16 tidev/node-ios-device#71

Merged

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

src: make AtExit callback's per Environment #9163

src: make AtExit callback's per Environment #9163

Conversation

danbev commented Oct 18, 2016

Checklist

Affected core subsystem(s)

Description of change

danbev commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Oct 18, 2016

bnoordhuis commented Oct 18, 2016 • edited Loading

danbev commented Oct 19, 2016

danbev commented Oct 19, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Oct 20, 2016

danbev commented Oct 25, 2016

danbev commented Nov 3, 2016

bnoordhuis commented Nov 11, 2016

danbev commented Nov 11, 2016

MylesBorins commented Sep 19, 2017

gibfahn commented Jan 17, 2018

danbev commented Jan 18, 2018

danbev commented Jan 22, 2018

bnoordhuis commented Oct 18, 2016 •

edited

Loading