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: some napi_*_threadsafe_function naming/documentation issues #25341

Closed
DaAitch opened this issue Jan 4, 2019 · 3 comments
Closed

NAPI: some napi_*_threadsafe_function naming/documentation issues #25341

DaAitch opened this issue Jan 4, 2019 · 3 comments
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.

Comments

@DaAitch
Copy link
Contributor

DaAitch commented Jan 4, 2019

1. initial_thread_count parameter of napi_create_threadsafe_function

I think the name and documentation of parameter initial_thread_count in napi_create_threadsafe_function is very confusing.

Without reading node impl, for me it was not possible understand what it does. In context of the methods napi_acquire/release_threadsafe I think it should be better called sth. like initial_acquired, so it is more clear, that that's the corresponding amount of napi_acquire_threadsafe_function calls.

If we don't/can't change the name, at least documentation should be adapted as it's misleading:

[in] initial_thread_count: The initial number of threads, including the main thread, which will be making use of this function.

maybe (simply) better something like this:

[in] initial_acquired: The initial number of acquisitions.

2. documention threadsafe chapter

For me the header documentation of the threadsafe chapter is too long, while there is less documentation in the functions.

I'd like to read something like this to understand how it works from top:

...
A threadsafe_function has two different kinds of reference counting that
prevents it from being destroyed:
  1. environment reference via `napi_ref/unref_threadsafe_function` (to
     be only called from node thread)
  2. acquisition via `napi_acquire/release_threadsafe_function` (to be
     called from any thread)

It is destroyed, either if:
  1. ref-count via `napi_ref/unref_threadsafe_function` is zero and all
     acquisitions are released
  2. or it's released via `napi_tsfn_abort` mode`

Maybe some more specific documentation in the functions and the parameters plus examples.

Went through it and found this, which is both wrong I think? not a napi_value and not referenced.

napi_create_threadsafe_function() creates a persistent reference to a napi_value

Maybe it should be updated.

What do you think?

@mhdawson
Copy link
Member

mhdawson commented Jan 4, 2019

I agree both of those can be clarified. In what you have in section 1, it should probably have a link to the threadsafe chapter so that people can better understand what initial_acquired would mean.

@targos targos added doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. node-api Issues and PRs related to the Node-API. labels Jun 13, 2020
@targos
Copy link
Member

targos commented Jun 13, 2020

@nodejs/n-api

@legendecas
Copy link
Member

Closing since #33871 has been landed. Feel free to reopen if there are any other thoughts we can continue to work on.

@legendecas legendecas removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants