Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: get Node API version used by addon #45715
node-api: get Node API version used by addon #45715
Changes from 14 commits
bbb0fb5
c8c6f56
4c11c92
272ba92
f297f67
b4ae2a6
fe46dee
3748a4e
bd38aaf
ac53d6b
f10f20f
bb3cc12
4b709c1
ad50859
38d5f0f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This does match current behaviour but also means that we'll have a leak right? Since we can change behaviour in a new Node-API version without affecting existing applications maybe the right thing to do is to consider all symbols as not being week so that the leak does not occur?
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.
@mhdawson, there are no memory leaks. The only side effect is that for the global and well-known symbols we always return a valid symbol because they are global and never collected by GC (it is by design). The local symbols are behaving the same way as weak references to objects.
We say that when the
napi_ref
ref count goes to zero we are converting values to weak references if they are supported, otherwise we release them. Symbols support weak references, but they do it partially: for some of them the weak reference will always return "live" value because GC never collects them.There is also a difference in the JavaScript
WeakRef
behavior. It throws an exception if a global symbol is given to it but succeeds for local and well-known symbols. We can align with this behavior in future when V8 API lets us to see the symbol type. Though we should probably avoid throwing an exception and thus resetting the global symbols instead of keeping them could be not that big of a deal.In any case to avoid native memory leaks we require to call
napi_delete_reference
. Changing the ref count was never controlling the memory lifetime as we see it in COMIUnknown
or instd::shared_ptr
. If thenapi_delete_reference
is not called explicitly, then the native memory will be removed onnapi_env
finalization. So, technically it is also not a memory leak, but rather delays the memory release.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.
@vmoroz thanks for the clarification. I was thinking about the auto-delete we have in RefBase but looking more closely I see that won't be related to direct use of references.