n-api: handle reference delete before finalize by mhdawson · Pull Request #24494 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Crashes were reported during finalization due to
the memory for a reference being deleted and the
finalizer running after the deletion.
This change ensures the deletion of the memory for
the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes- commit message follows commit guidelines
@mhdawson sadly an error occured when I tried to trigger a build :(
Issues and PRs that require attention from people who are familiar with C++.
Issues and PRs related to the Node-API.
labels
@hashseed would be good to get somebody from the V8 team to comment on the issue as well since it new gc behaviour seems to be the trigger for us finding the issue.
@gabrielschulhof I know you had run into some issues that resulted in the original code to handle the Reference being deleted in the callback. Would be good to get your review as well.
This looks like it already needs a rebase? Plus, it might be nice to have a test for this or so…
Rebased. Was planning to look at a test but I likely won't get to that later in the week.
@mhdawson commit message has deletion misspelled as "deleteion"
Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion.
This change ensures the deletion of the memory for the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Ok, test added. Validated that the test fails consistently without the fix and passes with it :) @addaleax should be good to go now.
and thanks to @toddwong for the basis of the test. I'm surprised it was straightforward to have a test that recreates relatively easily and that runs in a short time period.
CI run was good, will plan to land tomorrow unless there are any additional comments/objections.
@mhdawson I'm assuming you didn't mean to close this?
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
@mhdawson Ahh! Pleased to know that was helping 😄 And thanks for the fix!
This was referenced
Dec 7, 2018
@mhdawson could you potentially backport this to v10.x? I've added a label, but feel free to remove it!
refack pushed a commit to refack/node that referenced this pull request
Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion.
This change ensures the deletion of the memory for the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
PR-URL: nodejs#24494 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Franziska Hinkelmann franziska.hinkelmann@gmail.com Reviewed-By: Refael Ackermann refack@gmail.com
mhdawson added a commit to mhdawson/io.js that referenced this pull request
Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion.
This change ensures the deletion of the memory for the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Backport-PR-URL: nodejs#25572 PR-URL: nodejs#24494 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Franziska Hinkelmann franziska.hinkelmann@gmail.com Reviewed-By: Refael Ackermann refack@gmail.com
@codebytere thanks for reminding me on this one, it is important to get back to 10.x and I should have thought of getting it done earlier.
mhdawson added a commit to mhdawson/io.js that referenced this pull request
Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion.
This change ensures the deletion of the memory for the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Backport-PR-URL: nodejs#25574 PR-URL: nodejs#24494 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Franziska Hinkelmann franziska.hinkelmann@gmail.com Reviewed-By: Refael Ackermann refack@gmail.com
BethGriggs pushed a commit that referenced this pull request
Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion.
This change ensures the deletion of the memory for the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Backport-PR-URL: #25574 PR-URL: #24494 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Franziska Hinkelmann franziska.hinkelmann@gmail.com Reviewed-By: Refael Ackermann refack@gmail.com
rvagg pushed a commit that referenced this pull request
Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion.
This change ensures the deletion of the memory for the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Backport-PR-URL: #25574 PR-URL: #24494 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Franziska Hinkelmann franziska.hinkelmann@gmail.com Reviewed-By: Refael Ackermann refack@gmail.com
mhdawson added a commit to mhdawson/io.js that referenced this pull request
nodejs#24494 fixed a crash but resulted in increased stress on gc finalization. A leak was reported in nodejs#26667 which we are still investigating. As part of this investigation I realized we can optimize to reduce amount of deferred finalization. Regardless of the root cause of the leak this should be a good optimization. It also resolves the leak for the case being reported in nodejs#26667. The OP in 26667 has confirmed that he can still recreate the original problem that 24494 fixed and that the fix still works with this optimization
danbev pushed a commit that referenced this pull request
#24494 fixed a crash but resulted in increased stress on gc finalization. A leak was reported in #26667 which we are still investigating. As part of this investigation I realized we can optimize to reduce amount of deferred finalization. Regardless of the root cause of the leak this should be a good optimization. It also resolves the leak for the case being reported in #26667. The OP in 26667 has confirmed that he can still recreate the original problem that 24494 fixed and that the fix still works with this optimization
PR-URL: #27085 Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
This was referenced
May 3, 2019
Reviewers
Labels
PRs that have at least one approval, no pending requests for changes, and a CI started.
Issues and PRs that require attention from people who are familiar with C++.
Issues and PRs related to the Node-API.