n-api: do not require JS Context for `napi_async_destroy()` #27255

addaleax · 2019-04-16T11:23:41Z

src: add `Environment` overload of `EmitAsyncDestroy`

This can be necessary for being able to call the function when no
JS Context is on the stack, e.g. during GC.

(This is technically semver-minor.)

n-api: do not require JS Context for `napi_async_destroy()`

Allow the function to be called during GC, which is a common use case.

Fixes: #27218

/cc @nodejs/n-api

src: do not require JS Context for `~AsyncResoure()`

Allow the destructor to be called during GC, which is a common use case.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This can be necessary for being able to call the function when no JS Context is on the stack, e.g. during GC. Refs: nodejs#27218

nodejs-github-bot · 2019-04-16T11:23:43Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3272

Allow the function to be called during GC, which is a common use case. Fixes: nodejs#27218

Allow the destructor to be called during GC, which is a common use case.

test/node-api/test_make_callback/binding.c

nodejs-github-bot · 2019-04-16T11:48:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22488/

mhdawson

LGTM

addaleax · 2019-04-16T20:30:41Z

By the way, it sounds like there may not be any more v8.x semver-minor releases. (/cc @MylesBorins @nodejs/lts)

I do think that this, as a bug fix, should end up in a v10.x and v8.x release, respectively, according to the usual rules for bug fixes. However, the first commit here is technically semver-minor. So, that leaves a few options:

Remove the semver-minor label. This makes this fit in well with our LTS workflow, is super-simple, but is also just plain wrong in terms of what the relevant commit does – it adds an API.
Make the added function overload purely internal in backports. This is also easy, but seems to go against the best interests of our users, because they may need access to a function like that in the same way that we use the added function here internally. (It might be worth noting that this functionality isn’t all that widely used, likely.)
Convince the LTS team to do another v8.x semver-minor release. I would expect that this is, unlike the other options, not an easy thing to do.

Any preferences?

addaleax · 2019-04-24T19:54:43Z

Landed in #27255

This can be necessary for being able to call the function when no JS Context is on the stack, e.g. during GC. Refs: #27218 PR-URL: #27255 Fixes: #27218 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Allow the function to be called during GC, which is a common use case. Fixes: #27218 PR-URL: #27255 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Allow the destructor to be called during GC, which is a common use case. PR-URL: #27255 Fixes: #27218 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson · 2019-04-26T17:50:14Z

From the options I think 2 or 3 is the right answer. I'm ok with 2 but it does seem odd to do extra work to hide the functionality just so that it can go into a semver minor release. That is unless there is more overall work in doing a release as semver minor versus semver patch. I do know that the 'extra' work might be to discuss what other semver minors might also go in but if we could just agree that the next release would be be semver minor with this one minor plus the other 'patches' then it might not be any additional work.

This can be necessary for being able to call the function when no JS Context is on the stack, e.g. during GC. Refs: #27218 PR-URL: #27255 Fixes: #27218 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Allow the function to be called during GC, which is a common use case. Fixes: #27218 PR-URL: #27255 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Allow the destructor to be called during GC, which is a common use case. PR-URL: #27255 Fixes: #27218 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Notable changes: * intl: * Update ICU to 64.2. This adds support for Japanese Era (Reiwa). #27361 * Fixes a bug in ICU that affected Node.js 12.0.0 in the case where `new Date().toLocaleString()` was called with a non-default locale. #27415 * C++ API: * Added an `Environment` overload of `EmitAsyncDestroy`. #27255 PR-URL: TODO

Notable changes: * intl: * Update ICU to 64.2. This adds support for Japanese Era (Reiwa). #27361 * Fixes a bug in ICU that affected Node.js 12.0.0 in the case where `new Date().toLocaleString()` was called with a non-default locale. #27415 * C++ API: * Added an overload of `EmitAsyncDestroy` that can be used during garbage collection. #27255 PR-URL: #27440

src: add Environment overload of EmitAsyncDestroy

7947f48

This can be necessary for being able to call the function when no JS Context is on the stack, e.g. during GC. Refs: nodejs#27218

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. node-api Issues and PRs related to the Node-API. lts-watch-v8.x labels Apr 16, 2019

addaleax requested a review from bnoordhuis April 16, 2019 11:23

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 16, 2019

addaleax added 2 commits April 16, 2019 13:37

n-api: do not require JS Context for napi_async_destroy()

4128307

Allow the function to be called during GC, which is a common use case. Fixes: nodejs#27218

src: do not require JS Context for ~AsyncResoure()

66f6944

Allow the destructor to be called during GC, which is a common use case.

addaleax force-pushed the napi-async-destroy branch from 75cdfaf to 66f6944 Compare April 16, 2019 11:37

bnoordhuis approved these changes Apr 16, 2019

View reviewed changes

test/node-api/test_make_callback/binding.c Show resolved Hide resolved

addaleax changed the title ~~n-api: do not requires JS Context for napi_async_destroy()~~ n-api: do not require JS Context for napi_async_destroy() Apr 16, 2019

fixup! n-api: do not require JS Context for napi_async_destroy()

f880b4f

jasnell approved these changes Apr 16, 2019

View reviewed changes

mhdawson approved these changes Apr 16, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019

addaleax closed this Apr 24, 2019

addaleax deleted the napi-async-destroy branch April 24, 2019 19:54

targos mentioned this pull request Apr 27, 2019

v12.1.0 proposal #27440

Merged

BethGriggs added the backport-requested-v10.x label Oct 17, 2019

legendecas mentioned this pull request Oct 16, 2020

n-api: napi_make_callback emit async init event with resource of async_context #32930

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: do not require JS Context for `napi_async_destroy()` #27255

n-api: do not require JS Context for `napi_async_destroy()` #27255

addaleax commented Apr 16, 2019 •

edited

Loading

nodejs-github-bot commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

mhdawson left a comment

addaleax commented Apr 16, 2019 •

edited

Loading

addaleax commented Apr 24, 2019

mhdawson commented Apr 26, 2019

n-api: do not require JS Context for napi_async_destroy() #27255

n-api: do not require JS Context for napi_async_destroy() #27255

Conversation

addaleax commented Apr 16, 2019 • edited Loading

src: add Environment overload of EmitAsyncDestroy

n-api: do not require JS Context for napi_async_destroy()

src: do not require JS Context for ~AsyncResoure()

Checklist

nodejs-github-bot commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

mhdawson left a comment

Choose a reason for hiding this comment

addaleax commented Apr 16, 2019 • edited Loading

addaleax commented Apr 24, 2019

mhdawson commented Apr 26, 2019

n-api: do not require JS Context for `napi_async_destroy()` #27255

n-api: do not require JS Context for `napi_async_destroy()` #27255

addaleax commented Apr 16, 2019 •

edited

Loading

src: add `Environment` overload of `EmitAsyncDestroy`

n-api: do not require JS Context for `napi_async_destroy()`

src: do not require JS Context for `~AsyncResoure()`

addaleax commented Apr 16, 2019 •

edited

Loading