src: make AtExit callback's per Environment by danbev · Pull Request #9163 · nodejs/node · GitHub

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

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

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.

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.

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

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?

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 :)

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.

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

Great, I'll move them.

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

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

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.

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 You can use uv_key_t instead

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

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

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.

Unnecessary whitespace change.

I'll remove this.

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.

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

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.

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?

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

@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 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 <jasnell@gmail.com> PR-URL: #12450 Reviewed-By: João Reis <reis@janeasystems.com>

This is not landing cleanly on v7.x-staging. Mind submitting a backport PR?

@evanlucas I'll try to take a look at this tomorrow

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 <jasnell@gmail.com> PR-URL: #12450 Reviewed-By: João Reis <reis@janeasystems.com>

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 Ref: nodejs#9163 Reviewed-By: James M Snell <jasnell@gmail.com>

This commit attempts to address one of the TODOs in nodejs#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. PR-URL: nodejs#9163 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

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

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.

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

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.

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

danbev deleted the at-exit-per-environment branch April 20, 2017 07:51

evanlucas added the backport-requested-v7.x label Apr 25, 2017

danbev mentioned this pull request May 10, 2017

(v6.x backport) build: enable cctest to use generated objects #12948

Closed

2 tasks

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

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

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

src: make AtExit callback's per Environment by danbev · Pull Request #9163 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: make AtExit callback's per Environment - #9163

src: make AtExit callback's per Environment#9163danbev wants to merge 1 commit intonodejs:masternodejs/node:masterfrom danbev:at-exit-per-environmentdanbev/node:at-exit-per-environmentCopy head branch name to clipboard

Conversation

danbev commented Oct 18, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

danbev commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 18, 2016

Uh oh!

bnoordhuis commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danbev commented Oct 19, 2016

Uh oh!

danbev commented Oct 19, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Oct 20, 2016

Uh oh!

danbev commented Oct 25, 2016

Uh oh!

danbev commented Nov 3, 2016

Uh oh!

bnoordhuis commented Nov 11, 2016

Uh oh!

danbev commented Nov 11, 2016

Uh oh!

evanlucas commented Apr 25, 2017

src: make AtExit callback's per Environment#9163
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:at-exit-per-environment

bnoordhuis commented Oct 18, 2016 •
edited
Loading