Add example/api to use ThreadSafeFunction with optional callback param

[legendecas]

Since nodejs/node#27791 has been landed, there might be some use cases on node-addon-api to use Napi::ThreadSafeFunction with optional callback parameter.

Approaches might be adding an example of how to use existing Napi::ThreadSafeFunction::New like:

Function callback;
auto tsfn = ThreadSafeFunction::New(env, callback, resource_name, 1, 1);

(is there a more intuitive one?)

Or add a new API which make the callback omitted:

auto tsfn = ThreadSafeFunction::New(env, resource_name, 1, 1);

[KevinEady]

I think it would make more sense to add a new signature without the Function argument. I feel like that's more intuitive. We are currently working on a fix to add a copy constructor, so odds are we could add this as well.

[KevinEady]

Sooo I think implementing a new constructor will blow up the number of constructors we have. We currently have thirteen ::New overloads, and if making the Function optional by removing it, we would add thirteen more. I dunno, we will discuss in next napi meeting

[KevinEady]

I would like to use this issue to highlight all issues with TSFN constructors. I point out the ambiguity between the TSFN::New with Finalizer + FinalizerDataType and ContextType + Finalizer:

struct TestData {
  // Native Promise returned to JavaScript
  Promise::Deferred deferred;

  // List of threads created for test. This list only ever accessed via main
  // thread.
  vector<thread> threads = {};

  ThreadSafeFunction tsfn = ThreadSafeFunction();
};

void FinalizerCallback(Napi::Env env, TestData* finalizeData){ 
  for (size_t i = 0; i < finalizeData->threads.size(); ++i) {   
    finalizeData->threads[i].join();                            
  }                                                             
  finalizeData->deferred.Resolve(Boolean::New(env,true));       
  delete finalizeData;
}

static Value TestAcquire(const CallbackInfo& info) { 
 // ....
  TestData *testData = new TestData({
    Promise::Deferred::New(env)
  });

  testData->tsfn = ThreadSafeFunction::New(env, cb, "Test", 0, 1, 
    FinalizerCallback, testData);
// ...
}

The call to New is ambiguous:

./threadsafe_function/threadsafe_function_sum.cc:142:20: error: call to 'New' is ambiguous
  testData->tsfn = ThreadSafeFunction::New(env, cb, "Test", 0, 1, FinalizerCallback, testData);
                   ^~~~~~~~~~~~~~~~~~~~~~~
/Users/kevineady/Documents/Projects/node-addon-api/napi-inl.h:3839:47: note: candidate function [with ResourceString = const char *, Finalizer = void
      (*)(Napi::Env, (anonymous namespace)::TestData *), FinalizerDataType = (anonymous namespace)::TestData]
inline ThreadSafeFunction ThreadSafeFunction::New(napi_env env,
                                              ^
/Users/kevineady/Documents/Projects/node-addon-api/napi-inl.h:3852:47: note: candidate function [with ResourceString = const char *, ContextType = void
      (Napi::Env, (anonymous namespace)::TestData *), Finalizer = (anonymous namespace)::TestData *]
inline ThreadSafeFunction ThreadSafeFunction::New(napi_env env,
                                              ^
1 error generated.
make: *** [Debug/obj.target/binding/threadsafe_function/threadsafe_function_sum.o] Error 1

I can solve this issue by explicitly using std::function like so:

testData->tsfn = ThreadSafeFunction::New(env, cb, "Test", 0, 1, 
  std::function<void (Napi::Env env, TestData* finalizeData)>(FinalizerCallback), 
  testData);

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[mhdawson]

@KevinEady , @legendecas should do we still need this issue?

[KevinEady]

@mhdawson , is it useful to provide an example with the optional callback..? Not sure, because it is as simple as not providing the Function& parameter... 🤷

[mhdawson]

@legendecas what's your take, I don't have a strong opinion one way or the other.

[legendecas]

I may say that this may not be the way ThreadSafeFunction recommended to use. I'm closing since I did not find any strong use case on this. Even cases may arise, documenting that the callback can be empty sounds enough to me.