-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
node-api: allow napi_delete_reference in basic finalizers #55620
base: main
Are you sure you want to change the base?
The head ref may contain hidden characters: "node-api/delete\u2013reference"
Conversation
`napi_delete_reference` must be called immediately for a `napi_reference` returned from `napi_wrap` in the corresponding finalizer, in order to clean up the `napi_reference` timely. `napi_delete_reference` is safe to be invoked during GC.
Review requested:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55620 +/- ##
==========================================
- Coverage 88.43% 87.96% -0.47%
==========================================
Files 654 656 +2
Lines 187666 188384 +718
Branches 36119 35968 -151
==========================================
- Hits 165954 165716 -238
- Misses 14954 15838 +884
- Partials 6758 6830 +72
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@vmoroz will confirm that there is no issue in a Hermes implementation. Main concern is around if deleting the reference and making an object collectable could cause some issues, for example if an engine uses reference counting and would try to collect the object immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as request changes so we don't land until the check @vmoroz is going to do is complete
@@ -0,0 +1,20 @@ | |||
// Flags: --expose-gc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also test that the finalizer was called? Like, adding a heap-allocated int
to the environment with napi_set_instance_data
which is incremented from the finalizer, and an accessor to ensure that the value was incremented. We should try to make sure, if possible, that we check after gc but before the event loop has had a chance to run. That way we can distinguish between synchronous and asynchronous finalization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks!
napi_delete_reference
must be called immediately for anapi_reference
returned fromnapi_wrap
in the correspondingfinalizer, in order to clean up the
napi_reference
timely.napi_delete_reference
is safe to be invoked during GC.