-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
test,n-api: delete strong ref w/o unrefing first #34822
test,n-api: delete strong ref w/o unrefing first #34822
Conversation
It's great that this fixes the test, but it still seems counterintuitive that unref'ing a reference can lead to a memory leak? I would not say that this "Fixes:" the issue. |
@addaleax you're right. We should either document that strong references must not be weakened before getting deleted or figure out how to accommodate weakening them before deletion. I'll replace the "Fixes" with "Re". |
Re: nodejs#34731 Signed-off-by: Gabriel Schulhof <[email protected]>
a24d6ed
to
2a8342e
Compare
I agreed and unless there is some V8 level limitation I think it should be |
I'd also lean towards not landing this until we better understand the issue, as it provides the reminder that we need to fix the underlying problem. |
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.
I'm personally okay with landing this, if you like.
(Edit: Btw, we use Refs:
for referencing issues in commit messages almost universally at this point. The commit message linter might expect that as well, I’m not sure.)
Fixes: #34731
Signed-off-by: @gabrielschulhof
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes