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

napi_create_threadsafe_function func should be optional #27592

Closed
josephg opened this issue May 7, 2019 · 5 comments
Closed

napi_create_threadsafe_function func should be optional #27592

josephg opened this issue May 7, 2019 · 5 comments
Labels
node-api Issues and PRs related to the Node-API.

Comments

@josephg
Copy link
Contributor

josephg commented May 7, 2019

Short version:

The func parameter to napi_create_threadsafe_function(env, func, ...) should be optional if you pass a custom call_js_cb function. Not all uses of threadsafe_function use a static javascript function across all invocations.


I'm porting the foundationdb bindings to n-api. FDB creates a network thread on which it returns values. So, the normal usage is:

  • Javascript calls FDB function (main thread)
  • C code creates FDB future object (main thread)
  • FDB resolves the future and calls a callback (FDB network thread).
  • ???
  • Resolved future object is passed back to javascript (main thread)

I'm writing code to solve the missing piece. The current implementation creates a uv_async object each time. But that won't work with napi.

So I need to use napi_create_threadsafe_function(env, func, ...) to send the future value back to the main thread. Each time the future resolves (each item added to the queue) will resolve using its own callback function or future. For this reason I'm passing NULL as the func argument, and using my own resolver code.

But this makes the call to napi_create_threadsafe_function throw a napi_invalid_arg error! The func argument should to be optional if you specify your own call_js_cb callback. I'm working around it for now by making a junk function but thats ugly and gross.

The nodejs code is here, though we'll want to backport the equivalent change to node v10/11 as well. This change should be backwards compatible, because passing null was previously invalid in all cases.

@mhdawson
Copy link
Member

mhdawson commented May 9, 2019

@gabrielschulhof FYI. Making it optional seems reasonable to me if it is not required for all use cases.

@Fishrock123 Fishrock123 added the node-api Issues and PRs related to the Node-API. label May 14, 2019
@gabrielschulhof
Copy link
Contributor

@mhdawson sounds reasonable.

@legendecas
Copy link
Member

legendecas commented May 21, 2019

What about the func argument could be any JS value (undefined, object, anything)? Since there is no actual usage on the value except the case of no custom call_js_cb given. One might be concerning on the semantic change of napi_create_threadsafe_function.

@josephg
Copy link
Contributor Author

josephg commented May 21, 2019

Sounds fine to me!

@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 3, 2019

Closing, this was fixed in #27791.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Sep 2, 2019
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Sep 19, 2019
BethGriggs pushed a commit that referenced this issue Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

No branches or pull requests

6 participants