Skip to content

n-api: finalize during second-pass callback - #25992

Closed
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:25927-finalize-during-second-pass-callback
Closed

n-api: finalize during second-pass callback#25992
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:25927-finalize-during-second-pass-callback

Conversation

@gabrielschulhof

Copy link
Copy Markdown
Contributor

Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.

Fixes: #25927

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 7, 2019
@gabrielschulhofgabrielschulhof added the node-api Issues and PRs related to the Node-API. label Feb 7, 2019
Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.
Fixes: nodejs#25927
@gabrielschulhof
gabrielschulhofforce-pushed the 25927-finalize-during-second-pass-callback branch from 2fba9c8 to cf043b3CompareFebruary 7, 2019 20:24

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I assume this will need to be backported as well?

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

@mhdawson yep.

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

Landed in 6edf882.

gabrielschulhof pushed a commit that referenced this pull request Feb 11, 2019
Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.
Fixes: #25927
PR-URL: #25992
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof
gabrielschulhof deleted the 25927-finalize-during-second-pass-callback branch February 12, 2019 19:36
addaleax pushed a commit that referenced this pull request Feb 13, 2019
Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.
Fixes: #25927
PR-URL: #25992
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@targostargos mentioned this pull request Feb 14, 2019
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 4, 2019
Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.
Fixes: nodejs#25927
PR-URL: nodejs#25992
Backport-PR-URL: nodejs#26058
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 4, 2019
Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.
Fixes: nodejs#25927
PR-URL: nodejs#25992
Backport-PR-URL: nodejs#26060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this pull request Mar 12, 2019
Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.
Fixes: #25927
PR-URL: #25992
Backport-PR-URL: #26058
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this pull request Mar 20, 2019
Calling into the engine from a weak callback is unsafe, however, the
engine offers a way to attach a second-pass weak callback which gets
called when it is safe to call into JavaScript. This moves the point
at which the N-API finalize callback gets called to this latter point.
Fixes: #25927
PR-URL: #25992
Backport-PR-URL: #26060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 26, 2019
@BethGriggsBethGriggs mentioned this pull request May 1, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.node-apiIssues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

n-api: test for finalizer calls JS from first round phantom callback

5 participants

@gabrielschulhof@nodejs-github-bot@addaleax@cjihrig@mhdawson