ThreadSafeFunction(napi_threadsafe_function) works badly with BlockingCall()

[o2genum]

When a ThreadSafeFunction instance is created from napi_threadsafe_function, its BlockingCall method doesn't work properly:

• The passed callback is ignored, but still required by the method signature (the one supplied with napi_create_threadsafe_function is used)
• The passed data pointer is incorrect for some reason. napi_call_threadsafe_function works fine.

I'm doing this because I want to napi_unref_threadsafe_function without using all the verbose C APIs.

[KevinEady]

Hello @o2genum ,

Can you post some code that you are using? Here's some code using BlockingCall() in all three overloads:

node-addon-api/test/threadsafe_function/threadsafe_function.cc

Lines 52 to 66 in dd9fa8a

	auto callback = [](Env env, Function jsCallback, int* data) {
	jsCallback.Call({ Number::New(env, *data) });
	};
	switch (info->type) {
	case ThreadSafeFunctionInfo::DEFAULT:
	status = tsfn.BlockingCall();
	break;
	case ThreadSafeFunctionInfo::BLOCKING:
	status = tsfn.BlockingCall(&ints[index], callback);
	break;
	case ThreadSafeFunctionInfo::NON_BLOCKING:
	status = tsfn.NonBlockingCall(&ints[index], callback);
	break;
	}

We have two tests: one using BlockingCall with no callback or data, and one using BlockingCall with data and callback, passing an int* to callback, which receives that value.

node-addon-api/test/threadsafe_function/threadsafe_function_sum.cc

Lines 35 to 41 in e10e683

	void entryWithTSFN(ThreadSafeFunction tsfn, int threadId) {
	std::this_thread::sleep_for(std::chrono::milliseconds(std::rand() % 100 + 1));
	tsfn.BlockingCall( [=](Napi::Env env, Function callback) {
	callback.Call( { Number::New(env, static_cast<double>(threadId))});
	});
	tsfn.Release();
	}

And here we use BlockingCall() with only callback and no data.

[KevinEady]

Ahh, I am sorry, I misunderstood your issue. Using the node-addon-api with a pre-constructed napi_threadsafe_function fails. I will take a look. Thanks!

[o2genum]

Nice! By the way, I want to note I could have done without wrapping TSFN from C API into C++ API one if there was a way to call napi_unref_threadsafe_function from C++ API, to prevent the TSFN from keeping event loop alive.

[KevinEady]

Hi @o2genum ,

Yeah, so I'm not sure how we can wrap an existing napi_tsfn in a new ThreadSafeFunction... And yep, the main problem is with how the TSFN is not a "1:1 wrapper" with napi_tsfn functions I suppose.

When creating a node-addon-api TSFN, you do not specify a call_js_cb parameter like you do in underlying napi_create_threadsafe_function. Instead, this is passed as the optional Callback parameter in the [Non]BlockingCall method. (In the TSFN wrapper, this function is used, and the optional Callback and Data are passed as a wrapped function pointer inside the void* data pointer)

I'll need to discuss this with the napi team for a good approach / if it's even possible.

In the mean time, we have two PRs that can solve your problem:

• tsfn: Add wrappers for Ref and Unref #561: This adds wrappers for napi_ref_threadsafe_function / napi_unref_threadsafe_function directly
• tsfn: Implement copy constructor #546: By removing the unique pointer, we can add an operator napi_threadsafe_function. This will allow you to pass a wrapped TSFN directly to napi_unref_threadsafe_function

[KevinEady]

Hi @gabrielschulhof / @mhdawson ,

I wanted to bring this up at yesterday's meeting but alas...

To address this issue, I can think of two ways but both work off this general paradigm:

1. Store new instance property bool _isInternal
2. Create private ThreadSafeFunction(napi_threadsafe_function, bool isInternal) to track that it is an internally-created TSFN wrapper. This constructor is used within New
3. If developer calls existing public ThreadSafeFunction(napi_threadsafe_function), it would default isInternal to false
4. Use _isInternal inside [Non]BlockingCall and/or CallInternal

Now I say "two" ways because:
1. If we make this _isInternal a const bool on the wrapper, [I believe] we can use static asserts to throw a compile-time error when the with-callback [Non]BlockingCall method is used. However, if it is const, we can no longer use the assignment constructor to overwrite an existing TSFN with a different one, as the property cannot change.
2. Make it non-const, and throw a run-time error.

Edit: option 1 is not feasible

[KevinEady]

As discussed in 21-Oct meeting: The other existing [Non]BlockingCall() methods cover all use-cases for a node-addon-api created TSFN (no data, with function, with function+data). In order to handle this use-case of an already-existing napi_tsfn, we can create new method ThreadSafeFunction::[Non]BlockingCall(DataType* data) which imply to use data with the underlying napi_threadsafe_function directly. Can pass nullptr to use no data.