ThreadSafeFunction(napi_threadsafe_function) works badly with BlockingCall()

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.

@o2genum

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.

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!

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.

tsfn: Add wrappers for Ref and Unref

@o2genum

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

@gabrielschulhof

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:

Store new instance property bool _isInternal
Create private ThreadSafeFunction(napi_threadsafe_function, bool isInternal) to track that it is an internally-created TSFN wrapper. This constructor is used within New
If developer calls existing public ThreadSafeFunction(napi_threadsafe_function), it would default isInternal to false
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

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.

tsfn: add wrappers for Ref and Unref