Fundamental issues with ThreadSafeFunction

[gabrielschulhof]

The two issues below point to some fairly core problems with the current implementation of Napi::ThreadSafeFunction.

#653
#594

We may well have to create a new implementation, while at the same time retaining maximum backward compatibility and as smooth a transition path as possible for those who need the functionality mentioned in the issues above.

[KevinEady]

I have started working on #653. I will put any comments on that thread.

[KevinEady]

@gabrielschulhof / @legendecas / @mhdawson :

In response to this, I think a solution that can solve both problems above, is actually to create a new templated class ThreadSafeFunctionEx<Context> that:

• Getting rid of the "given-per-call" callback method signatures of [Non]BlockingCall.
  • This also addresses the concerns in Semantical difference between Napi::ThreadSafeFunction and napi_threadsafe_function #594 (comment) , and since this would be a new class, it will not break existing code. People can migrate to this ThreadSafeFunctionEx class when needed, or start developing using it immediately.
  • I think this would then inherently solve the above issue, because then the [Non]BlockingCall translates directly to a napi_call_threadsafe_function
• Getting rid of above makes it much simpler to fix Context parameter on thread safe function callback #653 which I've been struggling on for awhile
• And what about this, not sure though: Has only one New method, thereby putting the "what parameters are optional" into the logic of underlying napi_create_threadsafe_function Eg: if you do not want an async_resource, you would need to pass an empty Value(). This is for sake of keeping the code easily maintainable. (NB: need to figure out how nullptr context works, eg. maybe its ThreadSafeFunctionEx<nullptr_t>)
  • Could possibly default some arguments on the New method signature

I was originally against the creation of a new class but I just now saw the comment I linked above talking about removing those [Non]BlockingCall signatures and so maybe this is the best way forward... I should be on next weeks napi call to discuss.

Thanks, Kevin

[legendecas]

if you do not want an async_resource, you would need to pass an empty Value()

This reminds me that maybe we can introduce Maybe box into node-addon-api to provide a stronger nullable contract.

[mhdawson]

@KevinEady can we share most of the code between the existing ThreadSafeFunction and ThreadSafeFunctionEx<Context> ? If so it sounds like a good path to explore.

[KevinEady]

@KevinEady can we share most of the code between the existing ThreadSafeFunction and ThreadSafeFunctionEx<Context> ? If so it sounds like a good path to explore.

Hi @mhdawson , posted this to #687

[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 , @gabrielschulhof should we close this as fixed by #742?