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

n-api: keep napi_env alive while it has finalizers #31140

Closed
wants to merge 3 commits into from

Conversation

@addaleax
Copy link
Member

addaleax commented Dec 30, 2019

Manage the napi_env refcount from Finalizer instances, as the
finalizer may refer to the napi_env until it is deleted.

Fixes: #31134
Fixes: node-ffi-napi/node-ffi-napi#48

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Manage the napi_env refcount from Finalizer instances, as the
finalizer may refer to the napi_env until it is deleted.

Fixes: #31134
Fixes: node-ffi-napi/node-ffi-napi#48
@nodejs-github-bot

This comment has been minimized.

@Trott Trott dismissed their stale review Dec 30, 2019

test failures

@nodejs-github-bot

This comment has been minimized.

@legendecas

This comment has been minimized.

Copy link
Member

legendecas commented Jan 2, 2020

Just out of curiosity, why wrapped objects by napi_wrap do not surface this issue? I tested against objects been wrapped which has been attached to process object too, it didn't crash on exit.

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Jan 2, 2020

@legendecas Because those finalizers are called during the destruction of napi_env, so the napi_env is still accessible at that point, as I understand it.

@nodejs-github-bot

This comment has been minimized.

@legendecas

This comment has been minimized.

Copy link
Member

legendecas commented Jan 3, 2020

@addaleax Because those finalizers are called during the destruction of napi_env, so the napi_env is still accessible at that point, as I understand it.

Yeah, there are two lists tracking these references. Wondering if it is possible to track those array buffers in the list too?

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Jan 3, 2020

@addaleax Because those finalizers are called during the destruction of napi_env, so the napi_env is still accessible at that point, as I understand it.

Yeah, there are two lists tracking these references. Wondering if it is possible to track those array buffers in the list too?

Well … In an ideal world, I think it would have been cleaner and more consistent for N-API not to just blindly wrap the Node.js C++ Buffer API, and instead only provide the option to create a Buffer using napi_create_typedarray(env, napi_buffer, ...) analogously to napi_create_typedarray(env, napi_uint8_array, ...).

However, the way things currently work is that the N-API methods for dealing with buffers correspond to the C++ Buffer API. That has its own finalization mechanism.

The only way “out” of this that I could see is to try and implement the N-API buffer methods in terms of the regular N-API typed array methods – feel free to do that if you like, but it seems like that’s out of scope for this PR.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Jan 4, 2020

@Trott
Trott approved these changes Jan 4, 2020
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Jan 4, 2020

Landed in 493faf6

@addaleax addaleax closed this Jan 4, 2020
@addaleax addaleax deleted the addaleax:fix-napi-finalizer-env-ref branch Jan 4, 2020
addaleax added a commit that referenced this pull request Jan 4, 2020
Manage the napi_env refcount from Finalizer instances, as the
finalizer may refer to the napi_env until it is deleted.

Fixes: #31134
Fixes: node-ffi-napi/node-ffi-napi#48
PR-URL: #31140
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Jan 6, 2020
Manage the napi_env refcount from Finalizer instances, as the
finalizer may refer to the napi_env until it is deleted.

Fixes: #31134
Fixes: node-ffi-napi/node-ffi-napi#48
PR-URL: #31140
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
Manage the napi_env refcount from Finalizer instances, as the
finalizer may refer to the napi_env until it is deleted.

Fixes: #31134
Fixes: node-ffi-napi/node-ffi-napi#48
PR-URL: #31140
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.