node-api: force env shutdown deferring behavior #37303
Closed
Conversation
This file contains 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
nodejs-github-bot
added
the
c++
gabrielschulhof
force-pushed
the
node-api-double-free-ref2
branch
from
53f6c32
to
cd3e2d1
Compare
|
I think we should add this in the commit message too: |
|
@RaisinTen I'm not 100% sure that the crash is the same. In
|
|
@RaisinTen it sounds like @mhdawson tried to reproduce #36868 and was unable to do so. Therefore I think we should close #36868 unless the test in question remains flaky. |
mhdawson
reviewed
Feb 10, 2021
gabrielschulhof
commented
Feb 11, 2021
|
@gabrielschulhof I think @legendecas may be away for a few days and I'm thinking it might still be good to wait for him to have a look as well. What do you think? |
|
@mhdawson OK. |
|
@gabrielschulhof PR wise looks good to me. However, I may have been mistaken on the test case of #37236 (comment). I've tried the PR on my internal case and it does crash regardlessly. |
|
@legendecas if it still crashes, can you create a test case we can use here? You can even push it to my branch. |
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
gabrielschulhof
force-pushed
the
node-api-double-free-ref2
branch
from
6415c99
to
21e5b1d
Compare
|
Rebased. |
|
@legendecas P.S.: your original repro (https://github.com/legendecas/repro-napi-v8impl-refbase-double-free) no longer crashes with this PR. |
legendecas
approved these changes
Feb 18, 2021
There was a problem hiding this comment.
@gabrielschulhof it's true. This PR does fix a crash case. I'll try to dig out the stable re-production on the one with weak references.
gabrielschulhof
pushed a commit
that referenced
this pull request
Feb 19, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
|
Landed in 03806a0. |
targos
pushed a commit
that referenced
this pull request
Feb 28, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
gabrielschulhof
pushed a commit
to gabrielschulhof/node
that referenced
this pull request
Mar 6, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
gabrielschulhof
pushed a commit
to gabrielschulhof/node
that referenced
this pull request
Mar 19, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
gabrielschulhof
pushed a commit
to gabrielschulhof/node
that referenced
this pull request
Mar 19, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
gabrielschulhof
pushed a commit
to gabrielschulhof/node
that referenced
this pull request
Mar 26, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
targos
pushed a commit
to gabrielschulhof/node
that referenced
this pull request
Apr 24, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
targos
pushed a commit
that referenced
this pull request
Apr 26, 2021
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Backport-PR-URL: #37802 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Merged
legendecas
added a commit
to legendecas/node
that referenced
this pull request
Mar 29, 2022
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
richardlau
pushed a commit
that referenced
this pull request
Mar 30, 2022
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Backport-PR-URL: #42512 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The finalizer normally never gets called while a reference is strong.
However, during environment shutdown all finalizers must get called. In
order to unify the deferring behavior with that of a regular
finalization, we must force the reference to be weak when we call its
finalizer during environment shutdown.
Fixes: #37236
Co-authored-by: @legendecas