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

ObjectWrap destructors run after CleanupHook #45088

Closed
mmomtchev opened this issue Oct 20, 2022 · 4 comments · Fixed by #45903
Closed

ObjectWrap destructors run after CleanupHook #45088

mmomtchev opened this issue Oct 20, 2022 · 4 comments · Fixed by #45903
Labels
node-api Issues and PRs related to the Node-API.

Comments

@mmomtchev
Copy link
Contributor

mmomtchev commented Oct 20, 2022 •
edited
Loading

Version

16.17.0

Platform

Ubuntu 20.04

Subsystem

napi

What steps will reproduce the bug?

Having ObjectWrap objects when destroying the environment. May be related to having stale Persistent objects.

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

All C++ objects are destroyed before the CleanupHook

What do you see instead?

This is the stack trace

pymport-native.node!Napi::ObjectWrap<pymport::PyObj>::FinalizeCallback(napi_env env, void * data) (/home/mmom/src/pymport/node_modules/node-addon-api/napi-inl.h:4597)
v8impl::RefBase::Finalize(bool) (Unknown Source:0)
node_napi_env__::~node_napi_env__() (Unknown Source:0)
node::Environment::RunCleanup() (Unknown Source:0)
node::FreeEnvironment(node::Environment*) (Unknown Source:0)
node::NodeMainInstance::Run() (Unknown Source:0)
node::Start(int, char**) (Unknown Source:0)
libc.so.6!__libc_start_main(int (*)(int, char **, char **) main, int argc, char ** argv, int (*)(int, char **, char **) init, void (*)(void) fini, void (*)(void) rtld_fini, void * stack_end) (/build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308)
_start (Unknown Source:0)

Additional information

No response
@mmomtchev mmomtchev changed the title GC runs after CleanupHook ObjectWrap destructors run after CleanupHook Oct 20, 2022
@kvakil kvakil added the node-api Issues and PRs related to the Node-API. label Oct 23, 2022
@juanarbol
Copy link
Member

cc @nodejs/node-api

@legendecas
Copy link
Member

Currently, the document only defines that cleanup hooks are run in a reversed order, but no words on if the cleanup hooks are run before any other finalizers. As the status quo, all cleanup hooks are run before the finalizers, regardless of their registration order, as the napi_env's own cleanup is registered before the registration of any addon's cleanup hooks.

I believe it would be more reasonable to run cleanup hooks after pending finalizers are drained. After the cleanup hooks are invoked, there should be no callbacks can calling into the addon again.

@KevinEady
Copy link
Contributor

Hi @legendecas , could this issue also be related to TSFN "release in ObjectWrap destructor" crash stacktraces we are seeing in nodejs/node-addon-api#1109?

@legendecas
Copy link
Member

@KevinEady thanks for the link. At first sight, it seems like a finalization order problem too. I'll take a look at both issues.

@legendecas legendecas mentioned this issue Dec 18, 2022
Merged
@mmomtchev mmomtchev mentioned this issue Jan 18, 2023
Closed
nodejs-github-bot pushed a commit that referenced this issue Mar 20, 2023
@legendecas
node-api: document node-api shutdown finalization
2984cc3 
As status quo, the cleanup hooks are invoked before the `napi_finalize`
callbacks at the exit of Node.js environments. This gives addons a
chance to release their resource in a proper order manually.

Document this behavior explicitly to advocate the usage on cleanup
hooks instead of relying on the implied invocation of `napi_finalize`
callbacks at shutdown.

PR-URL: #45903
Fixes: #45088
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
RafaelGSS pushed a commit that referenced this issue Apr 5, 2023
@legendecas @RafaelGSS
node-api: document node-api shutdown finalization
7195f23 
As status quo, the cleanup hooks are invoked before the `napi_finalize`
callbacks at the exit of Node.js environments. This gives addons a
chance to release their resource in a proper order manually.

Document this behavior explicitly to advocate the usage on cleanup
hooks instead of relying on the implied invocation of `napi_finalize`
callbacks at shutdown.

PR-URL: #45903
Fixes: #45088
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
RafaelGSS pushed a commit that referenced this issue Apr 7, 2023
@legendecas @RafaelGSS
node-api: document node-api shutdown finalization
55f3d21 
As status quo, the cleanup hooks are invoked before the `napi_finalize`
callbacks at the exit of Node.js environments. This gives addons a
chance to release their resource in a proper order manually.

Document this behavior explicitly to advocate the usage on cleanup
hooks instead of relying on the implied invocation of `napi_finalize`
callbacks at shutdown.

PR-URL: #45903
Fixes: #45088
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
@legendecas @danielleadams
node-api: document node-api shutdown finalization
6e66371 
As status quo, the cleanup hooks are invoked before the `napi_finalize`
callbacks at the exit of Node.js environments. This gives addons a
chance to release their resource in a proper order manually.

Document this behavior explicitly to advocate the usage on cleanup
hooks instead of relying on the implied invocation of `napi_finalize`
callbacks at shutdown.

PR-URL: #45903
Fixes: #45088
Reviewed-By: James M Snell <[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
Assignees
No one assigned
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

node-api: document node-api shutdown finalization legendecas/node
5 participants
@legendecas @KevinEady @juanarbol @kvakil @mmomtchev