doc: document ThreadSafeFunction #494

KevinEady · 2019-06-13T14:13:55Z

gabrielschulhof · 2019-06-13T17:49:08Z

doc/threadsafe_function.md

@@ -0,0 +1,219 @@
+# ThreadSafeFunction
+
+JavaScript functions can normally only be called from a native addon's main thread. If an addon creates additional threads, then node-addon-api functions that require a `Env`, `Value`, or `Ref` must not be called from those threads.


The lines should be wrapped at 80.

"`Env`" and references to other classes should be namespaced with `Napi::`.

Added namespace of Napi, changed Ref to Reference

Lines have been wrapped to 80 characters.

gabrielschulhof · 2019-06-13T17:54:03Z

doc/threadsafe_function.md

+
+`ThreadSafeFunction` objects are destroyed when every thread which uses the object has called `Release()` or has received a return status of `CLOSING` in response to a call to `BlockingCall()` or `NonBlockingCall()`. The queue is emptied before the `ThreadSafeFunction` is destroyed. It is important that `Release()` be the last API call made in conjunction with a given `ThreadSafeFunction`, because after the call completes, there is no guarantee that the `ThreadSafeFunction` is still allocated. For the same reason it is also important that no more use be made of a thread-safe function after receiving a return value of `CLOSING` in response to a call to `BlockingCall()` or `NonBlockingCall()`. Data associated with the `ThreadSafeFunction` can be freed in its `Finalizer` callback which was passed to `ThreadSafeFunction::New()`.
+
+Once the number of threads making use of a `ThreadSafeFunction` reaches zero, no further threads can start making use of it by calling `Acquire()`. In fact, all subsequent API calls associated with it, except `Release()`, will return an error value of napi_closing.


"napi_closing" should be replaced with "`CLOSING`".

Removed all old references of the old implementation's ThreadSafeFunction::Status to the various napi_statuses

doc/threadsafe_function.md

gabrielschulhof · 2019-06-13T18:36:58Z

doc/threadsafe_function.md

+    Context* context);
+```
+
+- `env`: The `napi_env` environment in which to construct the `Napi::ThreadSafeFunction` object.


Ditto wrt. changing to Napi::Env.

gabrielschulhof · 2019-06-13T18:41:29Z

doc/threadsafe_function.md

+bool Napi::ThreadSafeFunction::Acquire()
+```
+
+Returns `true` if the thread can successfully be added to the thread count.


Maybe change this to ... if the thread has successfully acquired the thread-safe function for its use. The implication that a successful addition to the thread count implies that the thread may henceforth use this thread-safe function is not necessarily obvious.

The signature for Acquire(), Release(), Abort(), and BlockingCall()/NonBlockingCall() was changed to return an napi_status. This documentation has been updated for those return values.

gabrielschulhof · 2019-06-13T18:43:40Z

doc/threadsafe_function.md

+bool Napi::ThreadSafeFunction::Release()
+```
+
+Returns `true` if the thread-safe function has successfully released.


"... has been successfully released.", maybe?

Addressed; likewise with the signature change mentioned above.

gabrielschulhof · 2019-06-13T18:44:31Z

doc/threadsafe_function.md

+
+### Abort
+
+"Abort" the thread-safe function. This will cause all subsequent APIs associated with the thread-safe function except `Release()` to return `napi_closing` even before its reference count reaches zero. In particular, `BlockingCall` and `NonBlockingCall()` will return `napi_closing`, thus informing the threads that it is no longer possible to make asynchronous calls to the thread-safe function. This can be used as a criterion for terminating the thread. Upon receiving a return value of `napi_closing` from a threadsafe function call a thread must make no further use of the thread-safe function because it is no longer guaranteed to be allocated.


"`napi_closing`" should be "`CLOSING`".

napi_closing is correct as CLOSING was the previous ThreadSafeFunction::Status enum, which is no longer used.

gabrielschulhof · 2019-06-13T18:45:34Z

doc/threadsafe_function.md

+
+### BlockingCall / NonBlockingCall
+
+Calls the Javascript function in a either a blocking or non-blocking fashion.


"... in either a blocking ... "

gabrielschulhof · 2019-06-13T18:46:03Z

doc/threadsafe_function.md

+
+Calls the Javascript function in a either a blocking or non-blocking fashion.
+- `BlockingCall()`: the API blocks until space becomes available in the queue. Will never block if the thread-safe function was created with a maximum queue size of `0`.
+- `NonBlockingCall()`: will return `napi_queue_full` if the queue was full, preventing data from being successfully added to the queue


Period at the end of the sentence.

Re: nodejs#442

KevinEady · 2019-06-26T14:23:46Z

@gabrielschulhof / @romandev ,

Most recent commit addresses the issues brought up, with signatures now matching the approved #442 PR. Please review.

gabrielschulhof · 2019-06-26T23:57:19Z

doc/threadsafe_function.md

+will eventually be made to the JavaScript function.
+
+`Napi::ThreadSafeFunction` objects are destroyed when every thread which uses
+the object has called `Release()` or has received a return status of


They may also call Abort().

gabrielschulhof · 2019-06-26T23:59:20Z

doc/threadsafe_function.md

+the object has called `Release()` or has received a return status of
+`napi_closing` in response to a call to `BlockingCall()` or `NonBlockingCall()`.
+The queue is emptied before the `Napi::ThreadSafeFunction` is destroyed. It is
+important that `Release()` be the last API call made in conjunction with a given


.. or Abort() ...

One thing (even with the current wording, generally taken from n_api_asynchronous_thread_safe_function_calls): I don't think the text is correct, because Release() [or Abort()] don't necessarily have to be the last call.

In a normal, non-aborted flow, Release() would be the last call, but in an aborted flow, it is possible that a [Non]BlockingCall() could be the last call, as it also decreases the thread count:

This can be used as a criterion for terminating the thread.

Should we better specify the abortable behavior here as well, or is it fine to keep in the Abort section?

Release() or Abort() absolutely have to be the last call – on the thread making the call. I think the most important thing to point out, and the aspect to emphasize here is this: After you've called Release() or Abort(), make no further use of the TSFN on that thread, irrespective of how this will affect the other threads – namely that, in the case of Abort() it will cause them to receive napi_closings.

This latter point is documented further down in the paragraph, and BlockingCall() and NonBlockingCall() are documented to have this feature such that the two calls are presented side-by-side as equal and alternative ways of being informed of the TSFN's impending closure.

The fact is that the documentation in core is almost certainly not phrased ideally. This is actually an opportunity to improve both places of documentation.

mhdawson

LGTM

mhdawson · 2019-07-16T21:33:14Z

Just noticed that I think this might need a link on the main page: https://github.com/nodejs/node-addon-api

* doc: document ThreadSafeFunction PR-URL: nodejs/node-addon-api#494 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

doc: document ThreadSafeFunction

64a6847

Re: nodejs#442

gabrielschulhof reviewed Jun 13, 2019

View reviewed changes

doc: document ThreadSafeFunction

186ebf2

Re: nodejs#442

gabrielschulhof approved these changes Jun 27, 2019

View reviewed changes

mhdawson approved these changes Jun 28, 2019

View reviewed changes

mhdawson merged commit e9fa1ea into nodejs:master Jul 16, 2019

KevinEady mentioned this pull request Jul 17, 2019

doc: add ThreadSafeFunction to main README #513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: document ThreadSafeFunction #494

doc: document ThreadSafeFunction #494

KevinEady commented Jun 13, 2019

gabrielschulhof Jun 13, 2019

gabrielschulhof Jun 13, 2019

KevinEady Jun 26, 2019

KevinEady Jun 26, 2019

gabrielschulhof Jun 13, 2019

KevinEady Jun 26, 2019

gabrielschulhof Jun 13, 2019

gabrielschulhof Jun 13, 2019

KevinEady Jun 26, 2019

gabrielschulhof Jun 13, 2019

KevinEady Jun 26, 2019

gabrielschulhof Jun 13, 2019

KevinEady Jun 26, 2019

gabrielschulhof Jun 13, 2019

KevinEady Jun 26, 2019

gabrielschulhof Jun 13, 2019

KevinEady Jun 26, 2019

KevinEady commented Jun 26, 2019

gabrielschulhof Jun 26, 2019

gabrielschulhof Jun 26, 2019

KevinEady Jun 27, 2019

gabrielschulhof Jun 28, 2019

mhdawson left a comment

mhdawson commented Jul 16, 2019

		@@ -0,0 +1,219 @@
		# ThreadSafeFunction

		JavaScript functions can normally only be called from a native addon's main thread. If an addon creates additional threads, then node-addon-api functions that require a `Env`, `Value`, or `Ref` must not be called from those threads.


		`ThreadSafeFunction` objects are destroyed when every thread which uses the object has called `Release()` or has received a return status of `CLOSING` in response to a call to `BlockingCall()` or `NonBlockingCall()`. The queue is emptied before the `ThreadSafeFunction` is destroyed. It is important that `Release()` be the last API call made in conjunction with a given `ThreadSafeFunction`, because after the call completes, there is no guarantee that the `ThreadSafeFunction` is still allocated. For the same reason it is also important that no more use be made of a thread-safe function after receiving a return value of `CLOSING` in response to a call to `BlockingCall()` or `NonBlockingCall()`. Data associated with the `ThreadSafeFunction` can be freed in its `Finalizer` callback which was passed to `ThreadSafeFunction::New()`.

		Once the number of threads making use of a `ThreadSafeFunction` reaches zero, no further threads can start making use of it by calling `Acquire()`. In fact, all subsequent API calls associated with it, except `Release()`, will return an error value of napi_closing.


		### Abort

		"Abort" the thread-safe function. This will cause all subsequent APIs associated with the thread-safe function except `Release()` to return `napi_closing` even before its reference count reaches zero. In particular, `BlockingCall` and `NonBlockingCall()` will return `napi_closing`, thus informing the threads that it is no longer possible to make asynchronous calls to the thread-safe function. This can be used as a criterion for terminating the thread. Upon receiving a return value of `napi_closing` from a threadsafe function call a thread must make no further use of the thread-safe function because it is no longer guaranteed to be allocated.


		### BlockingCall / NonBlockingCall

		Calls the Javascript function in a either a blocking or non-blocking fashion.

doc: document ThreadSafeFunction #494

doc: document ThreadSafeFunction #494

Conversation

KevinEady commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinEady commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jul 16, 2019