Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
5 participants
@addaleax
Copy link
Member

commented Apr 16, 2019

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
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.

Refs: #27218
@nodejs-github-bot

This comment has been minimized.

addaleax added some commits Apr 16, 2019

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
src: do not require JS Context for `~AsyncResoure()`
Allow the destructor to be called during GC,
which is a common use case.

@addaleax addaleax force-pushed the addaleax:napi-async-destroy branch from 75cdfaf to 66f6944 Apr 16, 2019

@addaleax 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

@nodejs-github-bot

This comment has been minimized.

@mhdawson
Copy link
Member

left a comment

LGTM

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

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:

  1. 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.
  2. 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.)
  3. 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

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Landed in #27255

@addaleax addaleax closed this Apr 24, 2019

@addaleax addaleax deleted the addaleax:napi-async-destroy branch Apr 24, 2019

addaleax added a commit that referenced this pull request Apr 24, 2019

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.

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>

addaleax added a commit that referenced this pull request Apr 24, 2019

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

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>

addaleax added a commit that referenced this pull request Apr 24, 2019

src: do not require JS Context for `~AsyncResoure()`
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

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

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.

targos added a commit that referenced this pull request Apr 27, 2019

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.

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>

targos added a commit that referenced this pull request Apr 27, 2019

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

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>

targos added a commit that referenced this pull request Apr 27, 2019

src: do not require JS Context for `~AsyncResoure()`
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>

targos added a commit that referenced this pull request Apr 27, 2019

2019-04-29, Version 12.1.0 (Current)
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

@targos targos referenced this pull request Apr 27, 2019

Merged

v12.1.0 proposal #27440

targos added a commit that referenced this pull request Apr 29, 2019

2019-04-29, Version 12.1.0 (Current)
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

targos added a commit that referenced this pull request Apr 29, 2019

2019-04-29, Version 12.1.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.