n-api: handle reference delete before finalize #24494
Conversation
|
@mhdawson sadly an error occured when I tried to trigger a build :( |
nodejs-github-bot
added
c++
|
@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… |
|
Rebasing as we speak... |
mhdawson
force-pushed
the
finalizer-order2
branch
from
2d127df
to
e4cb28e
Compare
|
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
mhdawson
force-pushed
the
finalizer-order2
branch
from
e4cb28e
to
af87a67
Compare
|
@sam-github, thanks fixed. |
|
@gabrielschulhof that might help me with the test :) |
|
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? |
addaleax
added
the
author ready
|
@mhdawson Ahh! Pleased to know that was helping 😄 And thanks for the fix! |
|
@mhdawson could you potentially backport this to |
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>
|
@codebytere will add to my TODO list. |
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 backport PR as requested: #25574 |
|
@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. |
|
@codebytere PR for backport as requested: #25574 |
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>
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>
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>
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
#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>
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