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: make func argument of napi_create_threadsafe_function optional #27791

Closed
wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented May 21, 2019

Ref: #27592

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels May 21, 2019
@legendecas legendecas force-pushed the tsfn branch 2 times, most recently from a2bcde8 to dc0cb8e Compare May 21, 2019 06:19
@josephg
Copy link
Contributor

josephg commented May 21, 2019

Awesome - thanks for putting this together! <3

src/node_api.cc Outdated Show resolved Hide resolved
src/node_api.cc Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

@nodejs/n-api … how would one feature-detect this? Should we provide that?

@josephg
Copy link
Contributor

josephg commented May 21, 2019

That would be good - either that or it can be part of the standard API for napi version 5 and later.

src/node_api.cc Outdated Show resolved Hide resolved
src/node_api.cc Outdated Show resolved Hide resolved
src/node_api.cc Outdated Show resolved Hide resolved
test/node-api/test_threadsafe_function/binding.c Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor

@addaleax you could simply assume that passing func is optional and, if napi_create_threadsafe_function() returns napi_invalid_arg, you would know that you're on a version of N-API where func is mandatory.

I am curious as to the semverity of this change, though. If it's semver-minor, we need to go through the whole experimental life cycle, but I'm not sure how we would do that for a semantic change like this, rather than the clean addition of a new N-API.

@gabrielschulhof
Copy link
Contributor

@legendecas please also modify https://nodejs.org/docs/latest/api/n-api.html#n_api_napi_threadsafe_function_call_js by mentioning for the js_callback parameter that it can also be NULL if the thread-safe function was created without a JS callback.

@addaleax
Copy link
Member

@addaleax you could simply assume that passing func is optional and, if napi_create_threadsafe_function() returns napi_invalid_arg, you would know that you're on a version of N-API where func is mandatory.

Right, but you usually want to support as many versions as possible, so you’d still end up always providing func, because that’s the only thing you can practically do to resolve napi_invalid_arg, besides maybe erroring out, right?

If it's semver-minor, we need to go through the whole experimental life cycle

Is that a N-API-specific rule that’s written down somewhere? I would consider this semver-minor, but I agree that it doesn’t make sense to label this as experimental. (Fwiw, I think none of the N-API functions currently marked as experimental should be experimental, except maybe the threadsafe function feature due to its complexity.)

@gabrielschulhof
Copy link
Contributor

@addaleax you could simply assume that passing func is optional and, if napi_create_threadsafe_function() returns napi_invalid_arg, you would know that you're on a version of N-API where func is mandatory.

Right, but you usually want to support as many versions as possible, so you’d still end up always providing func, because that’s the only thing you can practically do to resolve napi_invalid_arg, besides maybe erroring out, right?

Well, if you get napi_invalid_arg, and knowing that the call should succeed, you would call again with a non-NULL func. But, of course, passing a dummy func is simplest.

My take is that this change is more about making napi_threadsafe_function into an async queue, which would clean up code that builds upon it, than it is about what to do with func.

If it's semver-minor, we need to go through the whole experimental life cycle

Is that a N-API-specific rule that’s written down somewhere? I would consider this semver-minor, but I agree that it doesn’t make sense to label this as experimental. (Fwiw, I think none of the N-API functions currently marked as experimental should be experimental, except maybe the threadsafe function feature due to its complexity.)

Well, the rule concerns new N-APIs. This isn't strictly a new N-API 🙂 But yeah, a semver-minor bump sounds like the way to go.

@gabrielschulhof gabrielschulhof added the semver-minor PRs that contain new features and should be released in the next minor version. label May 21, 2019
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

Excellent progress! A few minor changes and it'll be golden 👍

doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
src/node_api.cc Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member

I think we can be flexible on going through an experimental cycle. For new functions, I think it makes sense and we should stick to that. In this case I think we'll get agreement that we should just do this immediately. My preference would be to make the change, and bump the N-API version in the same PR. In that way you will know you can depend on the parameter being optional if it is version 5 or higher. We might also want to review if we should promote any other functions out of experimental at the same time as we bump the version number (being careful to think about whether those other changes can be backported in case we want to backport version 5 support to 10.x and 8.x)

@legendecas legendecas force-pushed the tsfn branch 2 times, most recently from 7bfb88f to b985dd2 Compare May 24, 2019 07:11
doc/api/n-api.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

Not actually requesting changes. On my part the PR is approved as is, however, I'm marking it as "Request changes" so as to prevent it from landing before we've had a chance to co-ordinate what else should be added to N-API 5.

@legendecas legendecas force-pushed the tsfn branch 2 times, most recently from bc653a9 to 4142960 Compare May 27, 2019 01:06
@Trott
Copy link
Member

Trott commented May 27, 2019

Not actually requesting changes. On my part the PR is approved as is, however, I'm marking it as "Request changes" so as to prevent it from landing before we've had a chance to co-ordinate what else should be added to N-API 5.

@gabrielschulhof Alternatively, you could approve it but add one of the blocked or wip labels. (But if a Request Changes is the best workflow for you, then by all means, do that.)

@gabrielschulhof gabrielschulhof dismissed their stale review May 28, 2019 20:51

Using the method mentioned by @Trott instead.

doc/api/n-api.md Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof added the wip Issues and PRs that are still a work in progress. label May 28, 2019
doc/api/n-api.md Outdated Show resolved Hide resolved
richardlau added a commit to richardlau/node-1 that referenced this pull request Jun 24, 2019
PR-URL: nodejs#28410
Refs: nodejs#27791
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
targos pushed a commit that referenced this pull request Jul 2, 2019
PR-URL: #28410
Refs: #27791
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Jul 2, 2019
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Sep 2, 2019
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Sep 19, 2019
BethGriggs pushed a commit that referenced this pull request Sep 20, 2019
@BethGriggs BethGriggs mentioned this pull request Oct 7, 2019
BethGriggs added a commit that referenced this pull request Oct 22, 2019
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 22, 2019
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
Sign up for free to 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-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants