Skip to content

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Dec 26, 2019

One napi_env is for each specific module in one context.

Related: #28682
Fixes: #31003

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label Dec 26, 2019
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 26, 2019
@legendecas legendecas added the doc Issues and PRs related to the documentations. label Dec 26, 2019
@legendecas
Copy link
Member Author

/cc @nodejs/n-api

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

doc/api/n-api.md Outdated
not allowed.
nested N-API calls. `napi_env` can be used through the context with the module,
that is to say, `napi_env` can not be used mixed up in [`Worker`][] threads,
and caching the `napi_env` for the purpose of general reuse is not allowed.
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

Maybe we should just change the last sentence to this:

"Caching the `napi_env` for the purpose of general reuse, and passing the napi_env between instances of the same addon running on different [`Worker`][] threads is not allowed. The napi_env becomes invalid when an instance of a native addon is unloaded. Notification of this event is delivered through the callbacks given to [`napi_add_env_cleanup_hook`][] and [`napi_set_instance_data`][]."

@BridgeAR
Copy link
Member

BridgeAR commented Jan 6, 2020

Ping @legendecas

@legendecas
Copy link
Member Author

@gabrielschulhof Updated :D

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

@mhdawson
Copy link
Member

mhdawson commented Jan 7, 2020

@legendecas looks like the linter failed. Can you review the failures and update.

@nodejs-github-bot
Copy link
Collaborator

gabrielschulhof pushed a commit that referenced this pull request Jan 8, 2020
PR-URL: #31102
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof
Copy link
Contributor

Landed in 96eceb7.

@legendecas legendecas deleted the napi_env branch January 8, 2020 23:30
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
PR-URL: #31102
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
codebytere pushed a commit that referenced this pull request Mar 14, 2020
PR-URL: #31102
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
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++. doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

N-API: Lifetime of napi_env is unclear
7 participants