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.

Sign up for GitHub

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

Jump to bottom

src: add support for addon instance data #663

Closed
wants to merge 1 commit into from
Closed

src: add support for addon instance data #663

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

Support napi_get_instance_data() and napi_set_instance_data().
Fixes: #654

legendecas
legendecas reviewed Jan 31, 2020
View reviewed changes
test/addon_data.cc Outdated Show resolved Hide resolved
addaleax
addaleax reviewed Jan 31, 2020
View reviewed changes
napi-inl.h Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof mentioned this pull request Jan 31, 2020
Closed
@tniessen
Copy link
Member

tniessen commented Feb 3, 2020

Is the implicit initialization really helpful? I would imagine most use cases to be one of these:

  1. For all calls into the module that require instance data, the module checks if instance data exists, and if it doesn't, it creates and initializes instance data. This PR takes away the ability to check if the instance data was newly created or if it previously existed.
  2. One call into the module creates and initializes instance data, all other calls only retrieve it. In this case, if due to a bug the instance data is not initialized correctly, the consuming functions have no way to check that, and since this PR silently creates a new object in the background, it might not even crash immediately. With separate APIs to set and get instance data, the consuming functions could just do the equivalent of CHECK on the returned pointer to make sure it has been initialized correctly.

To work around these issues, most objects would probably have to have a member is_initialized or something similar, instead of just checking whether the instance data is NULL or not. In all cases, requiring T to be default constructible is restrictive.

@gabrielschulhof gabrielschulhof mentioned this pull request Feb 4, 2020
Closed
3 tasks
@gabrielschulhof
Copy link
Contributor Author

@tniessen @legendecas @addaleax I changed the interface to an accessor pair as @tniessen suggested.

Landing this requires nodejs/node#31638 otherwise it crashes 😕

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Feb 6, 2020
n-api: free instance data as reference
38b4b6f 
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
gabrielschulhof pushed a commit to nodejs/node that referenced this pull request Feb 6, 2020
n-api: free instance data as reference
884e287 
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@codebytere
n-api: free instance data as reference
ae2141e 
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
codebytere pushed a commit to nodejs/node that referenced this pull request Mar 15, 2020
@codebytere
n-api: free instance data as reference
63bb634 
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
codebytere pushed a commit to nodejs/node that referenced this pull request Mar 17, 2020
@codebytere
n-api: free instance data as reference
9680fd4 
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@mhdawson mhdawson mentioned this pull request Mar 23, 2020
Closed
8 tasks
legendecas
legendecas reviewed Mar 25, 2020
View reviewed changes
napi-inl.h Outdated
@@ -322,6 +322,32 @@ inline Value Env::RunScript(String script) {
return Value(_env, result);
}

#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

I believe this guard can be replaced with NAPI_VERSION > 5?

mhdawson
mhdawson reviewed Mar 30, 2020
View reviewed changes
napi.h Outdated
@@ -173,6 +173,10 @@ namespace Napi {
///
/// In the V8 JavaScript engine, a N-API environment approximately corresponds to an Isolate.
class Env {
#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as before here as well with respect to NAPI_VERSION > 5 and the next one as well.

mhdawson
mhdawson approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once guards are updated.

codebytere pushed a commit to nodejs/node that referenced this pull request Mar 30, 2020
@codebytere
n-api: free instance data as reference
08e09ec 
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

@mhdawson @legendecas I have updated the guards.

legendecas
legendecas reviewed Apr 12, 2020
View reviewed changes
napi-inl.h Outdated
napi_status status =
napi_set_instance_data(_env, data, [](napi_env, void* data, void*) {
fini(static_cast<T*>(data));
}, nullptr);
Copy link
Member

@legendecas legendecas Apr 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just noted finalize hint has been hardcoded here. Is this intended?

@gabrielschulhof
Copy link
Contributor Author

@legendecas I added an option that takes a hint.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 24, 2020 •
edited
Loading

CI:

Version Job Status
v15.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2075/ ✗
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2076/ ✗
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2077/ ✓
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2078/ ✗

@gabrielschulhof
Copy link
Contributor Author

Investigating .o0o.o0o.

src: add support for addon instance data
a8959b6 
Support `napi_get_instance_data()` and `napi_set_instance_data()`.
Fixes: nodejs#654
@gabrielschulhof
Copy link
Contributor Author

The failure was caused by Windows' use of escape characters as path separators. The path to the addon was being passed through an unescape step.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 25, 2020 •
edited
Loading

CI:

Version Job Status
v15.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2079/ ✓
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2080/ ✓
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2081/ ✓
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2082/ ✓

gabrielschulhof pushed a commit that referenced this pull request Apr 25, 2020
src: add support for addon instance data
9c9accf 
Support `napi_get_instance_data()` and `napi_set_instance_data()`.

Signed-off-by: Gabriel Schulhof <[email protected]>
Fixes: #654
PR-URL: #663
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Landed in 9c9accf.

@KevinEady KevinEady mentioned this pull request Jun 22, 2020
Closed
@gabrielschulhof gabrielschulhof deleted the addon-class branch August 6, 2020 20:43
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
src: add support for addon instance data
006327e 
Support `napi_get_instance_data()` and `napi_set_instance_data()`.

Signed-off-by: Gabriel Schulhof <[email protected]>
Fixes: nodejs/node-addon-api#654
PR-URL: nodejs/node-addon-api#663
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
src: add support for addon instance data
41dd73f 
Support `napi_get_instance_data()` and `napi_set_instance_data()`.

Signed-off-by: Gabriel Schulhof <[email protected]>
Fixes: nodejs/node-addon-api#654
PR-URL: nodejs/node-addon-api#663
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
src: add support for addon instance data
8fb094a 
Support `napi_get_instance_data()` and `napi_set_instance_data()`.

Signed-off-by: Gabriel Schulhof <[email protected]>
Fixes: nodejs/node-addon-api#654
PR-URL: nodejs/node-addon-api#663
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
src: add support for addon instance data
b3ea89b 
Support `napi_get_instance_data()` and `napi_set_instance_data()`.

Signed-off-by: Gabriel Schulhof <[email protected]>
Fixes: nodejs/node-addon-api#654
PR-URL: nodejs/node-addon-api#663
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@mhdawson mhdawson mhdawson approved these changes

@tniessen tniessen Awaiting requested review from tniessen

@NickNaso NickNaso Awaiting requested review from NickNaso

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Best practice to store context-aware class constructor reference
5 participants
@gabrielschulhof @tniessen @addaleax @legendecas @mhdawson