Skip to content

Conversation

@legendecas
Copy link
Member

@legendecaslegendecas 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

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

/cc @nodejs/n-api

Copy link
Member

@NickNasoNickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gabrielschulhofgabrielschulhofDec 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

Ping @legendecas

@legendecas
Copy link
MemberAuthor

@gabrielschulhof Updated :D

Copy link
Member

@mhdawsonmhdawson 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

@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 <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof
Copy link
Contributor

Landed in 96eceb7.

@legendecaslegendecas 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 <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@codebyterecodebytere 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 <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@codebyterecodebytere mentioned this pull request Mar 17, 2020
Sign up for freeto 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++.docIssues and PRs related to the documentations.node-apiIssues 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

@legendecas@BridgeAR@mhdawson@nodejs-github-bot@gabrielschulhof@NickNaso@gengjiawen