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

[v12.x] src: render N-API weak callbacks as cleanup hooks #30070

Conversation

gabrielschulhof
Copy link
Contributor

Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the node::Environment and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: @addaleax
Reviewed-By: @mhdawson

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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++. v12.x labels Oct 22, 2019
@gabrielschulhof gabrielschulhof changed the title src: render N-API weak callbacks as cleanup hooks [v12.x] src: render N-API weak callbacks as cleanup hooks Oct 22, 2019
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Oct 22, 2019
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.

(As a note for reviewers, the contents are identical to those of the original PR)

@gabrielschulhof
Copy link
Contributor Author

Yeah, it was a clean cherry-pick.

Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: nodejs#28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof
Copy link
Contributor Author

I moved the addition of benchmark/napi/ref/.gitignore from the backport of #29995 (#30072) to this backport, because with it, #30072 will not pass make bench-addons-build && tools/test.py test/benchmark/test-benchmark-napi.js.

This way, at least one of the two PRs will pass that test, and, once they both land, the result should also pass.

@targos
Copy link
Member

targos commented Oct 23, 2019

Thanks for opening the PR, but if this is a clean cherry-pick, I'm a bit confused about why we don't land this change (along with #30072) in v12.x using the regular release process?

@gabrielschulhof
Copy link
Contributor Author

@targos I wanted to make sure it gets into v12.x because of the comments at nodejs/node-addon-api#567 (comment) and below.

@mhdawson
Copy link
Member

@gabrielf I think @targos just meant that they should be backported following this guidance: https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md

@targos
Copy link
Member

targos commented Nov 8, 2019

The original commit landed on v12.x-staging.

@targos targos closed this Nov 8, 2019
@gabrielschulhof gabrielschulhof deleted the backport-28428-to-v12.x branch March 13, 2020 06:47
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++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants