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: unload native addons when the environment is destroyed #24861

Closed

Conversation

gabrielschulhof
Copy link
Contributor

This is an alternative to #23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.

It includes the test/addons/worker-addon,
test/addons/hello-world/test-worker.js and the documentation update from the original PR written
by @addaleax in a separate commit.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 6, 2018
@gabrielschulhof gabrielschulhof added the addons Issues and PRs related to native addons. label Dec 6, 2018
src/node_binding.cc Outdated Show resolved Hide resolved
src/node_binding.h Show resolved Hide resolved
src/env-inl.h Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor Author

@joyeecheung I have addressed your review comments.

@bnoordhuis I have addressed your review comments. I put the body of DLib into a new node_binding-inl.h' which is included from the bottom of node_binding.h`.

src/node_binding-inl.h Outdated Show resolved Hide resolved
src/node_binding.cc Outdated Show resolved Hide resolved
addaleax and others added 3 commits December 6, 2018 15:31
This is an alternative to nodejs#23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you! 💙

src/env.cc Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

Failure looks legit.

@gabrielschulhof
Copy link
Contributor Author

Resume, just to see if it's 100% reproducible:

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

@addaleax
Copy link
Member

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Dec 15, 2018
@Trott
Copy link
Member

Trott commented Dec 16, 2018

Fresh CI rather than Resume Build so that it pulls in the fact that a bunch of tests are currently marked flaky: https://ci.nodejs.org/job/node-test-pull-request/19572/

@Trott
Copy link
Member

Trott commented Dec 16, 2018

Any chance that this AIX failure is related?

16:58:17 not ok 2208 addons/at-exit/test
16:58:17   ---
16:58:17   duration_ms: 0.723
16:58:17   severity: crashed
16:58:17   exitcode: -4
16:58:17   stack: |-
16:58:17   ...

I'm guessing not, but you tell me. ¯\(ツ)

In the meantime, Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19573/

@addaleax
Copy link
Member

@Trott I’m not sure whether we should wait for the outcome of the resume build (run stress tests if it comes back green?), but before/after stress tests with a hundred runs or so might be helpful?

@Trott
Copy link
Member

Trott commented Dec 16, 2018

before/after stress tests with a hundred runs or so might be helpful?

I guess if we're going to tie up an AIX machine for a few hours doing a stress test, this is the time of day/week to do it, so here we go:

Stress test on master: https://ci.nodejs.org/job/node-stress-single-test/2128/

Stress test on this PR: https://ci.nodejs.org/job/node-stress-single-test/2129/

@Trott
Copy link
Member

Trott commented Dec 16, 2018

CI Resume Build for AIX showed the same addons failure.

https://ci.nodejs.org/job/node-test-commit-aix/19762/nodes=aix61-ppc64/console

18:59:40 not ok 2208 addons/at-exit/test
18:59:40   ---
18:59:40   duration_ms: 0.918
18:59:40   severity: crashed
18:59:40   exitcode: -4
18:59:40   stack: |-
18:59:40   ...

Still awaiting stress test for this PR to start, but stress test on master is clean so far after almost 900 runs. (It's set to do 1000.)

Trott
Trott previously requested changes Dec 16, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Stress test is crashing on addons/at-exit/test 100% of the time after 400+ runs on AIX. Impressed/weirded-out that this issue is AIX only. But I guess it needs to be sorted out before landing...

20:22:34 not ok 1 addons/at-exit/test
20:22:34   ---
20:22:34   duration_ms: 0.573
20:22:34   severity: crashed
20:22:34   exitcode: -4
20:22:34   stack: |-
20:22:34   ...

@MylesBorins
Copy link
Contributor

This change broke the node-report module

It seems that is was somehow related to nan?

nodejs/node-report#116

Fix landed in nan and awaiting a release nodejs/nan#831

Would someone be willing to open a backport so we can track the progress?

@addaleax
Copy link
Member

This change broke the node-report module

@MylesBorins This seems very unlikely … can you double-check and maybe share how you tested this?

Fix landed in nan and awaiting a release nodejs/nan#831

That PR was opened before this one landed and the contents seem very much unrelated…

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 28, 2018
Originally from portions of nodejs#23319.

PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 28, 2018
This is an alternative to nodejs#23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.

PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Jan 1, 2019
Originally from portions of #23319.

Backport-PR-URL: #25258
PR-URL: #24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Jan 1, 2019
This is an alternative to #23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.

Backport-PR-URL: #25258
PR-URL: #24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Originally from portions of nodejs#23319.

PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This is an alternative to nodejs#23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.

PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
@BridgeAR
Copy link
Member

This has been backported and released in v11.7.0 but it turned out that this actually breaks node-report. Therefore I am going to remove the backport again and release v11.7.1 without this.
Please have another look at this so it does not cause any issues with node-report.

@addaleax
Copy link
Member

I’m sorry that I did not add a test case for the issue we’re seeing with node-report when I wrote the original PR at #23319 – I’ve opened #25577 to address it.

(This does not mean that this is not a bug in node-report, btw – it is. But there’s a lot of addons that run into similar issues with this PR, I assume.)

@richardlau
Copy link
Member

(This does not mean that this is not a bug in node-report, btw – it is. But there’s a lot of addons that run into similar issues with this PR, I assume.)

If it's something we can improve, please report/submit a PR over in https://github.com/nodejs/node-report. Note that one of the issues with addons and master is that nan requires an update to work with the version of V8 in master (AFAIK nodejs/nan#831 has yet to be released) which means addons currently fail in citgm, which is obviously masking stuff like this.

@addaleax
Copy link
Member

@richardlau I can try to do that, but given the work on integrating it into core in #22712, I don’t think I’d personally want to spend much time on the npm module… the first problem I encountered here is that nodereport_trigger_async is never removed from the event loop (e.g. in an Environment cleanup hook). Generally, in #22712 we had (and I still partly have) a hard time figuring out what lifetime management objects from node-report should have and how much global state is actually desirable and managable.

(Also, it’s absolutely understandable if you’re not interested in going through the nearly one thousand comments in #22712, but there’s a lot in there that applies to the npm module as well, including this issue iirc.)

addaleax added a commit to addaleax/node that referenced this pull request Jan 22, 2019
Unloading native addons from the main thread was an (presumably
unintended) significant breaking change, because addons may
rely on their memory being available after an `Environment` exits.

This patch only restricts this to Worker threads, at least for the
time being, and thus matches the behaviour from
nodejs#23319.

Refs: nodejs#24861
Refs: nodejs#23319
danbev pushed a commit that referenced this pull request Jan 23, 2019
Unloading native addons from the main thread was an (presumably
unintended) significant breaking change, because addons may
rely on their memory being available after an `Environment` exits.

This patch only restricts this to Worker threads, at least for the
time being, and thus matches the behaviour from
#23319.

PR-URL: #25577
Refs: #24861
Refs: #23319
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
addaleax added a commit that referenced this pull request Jan 23, 2019
Unloading native addons from the main thread was an (presumably
unintended) significant breaking change, because addons may
rely on their memory being available after an `Environment` exits.

This patch only restricts this to Worker threads, at least for the
time being, and thus matches the behaviour from
#23319.

PR-URL: #25577
Refs: #24861
Refs: #23319
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.