Context parameter on thread safe function callback #653

Activity

1. legendecas commented on Jan 18, 2020

   

   legendecas

   on Jan 18, 2020

   MemberAuthor

   More actions

   As a reference napi_threadsafe_function_call_js has a signature of

   typedef void (*napi_threadsafe_function_call_js)(napi_env env,
                                                    napi_value js_callback,
                                                    void* context,
                                                    void* data);
   
2. KevinEady commented on Jan 20, 2020

   

   KevinEady

   on Jan 20, 2020

   Last edited by KevinEady

   Contributor

   More actions

   Looking at the signature for our CallJS wrapper passed to napi_create_threadsafe_function:

   node-addon-api/napi-inl.h

   Lines 4572 to 4575 in 4e88506

   	inline void ThreadSafeFunction::CallJS(napi_env env,
   	napi_value jsCallback,
   	void* /* context */,
   	void* data) {

   ... we completely ignore the context value.

   The context is not wrapped, so one could use ThreadSafeFunction::GetContext() but it is an additional call that shouldn't be necessary.
3. legendecas commented on Jan 20, 2020

   

   legendecas

   on Jan 20, 2020

   Last edited by legendecas

   MemberAuthor

   More actions

   The context is not wrapped, so one could use ThreadSafeFunction::GetContext() but it is an additional call that shouldn't be necessary.

   I've considered this option too. It seems on the user callback we don't have the reference to call ThreadSafeFunction::GetContext() if we don't wrap it in the data. So the option still relies on wrapping around data.
4. gabrielschulhof commented on Feb 3, 2020

   

   gabrielschulhof

   on Feb 3, 2020

   Contributor

   More actions

   From #594 (comment) it looks like we may need to substantially revamp Napi::ThreadSafeFunction. When we do so, we should also address this issue.
5. KevinEady commented on Feb 16, 2020

   

   KevinEady

   on Feb 16, 2020

   Contributor

   More actions

   Sooo I have been working on this for some time now... After struggling with templated std::functions for hours, I really see two ways of implementing:

   1. Allow the existing [Non]BlockingCall()s to take a three-argument callback, with the context being void*.
   2. Create a new function [Non]BlockingCallEx which requires a three argument callback. I think I can make the context argument templated T *... but since the context is stored on the underlying napi_threadsafe_function and not the call itself, the node-addon-api wrapper to run the callback void(Env, Function, T*) would perform a cast to T* on the context, since it's void*.

   I dunno, I am leaning towards option 1... i'm sure there are other options but there was no way I could figure out how to "combine" the two options (modify the existing callbacks to allow templated argument third T* context)...

   Thoughts?

   cc: @gabrielschulhof @legendecas
6. github-actions commented on Dec 16, 2020

   

   github-actionsbot

   on Dec 16, 2020

   Contributor

   More 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.
7. github-actions

   added

   stale

   on Dec 16, 2020
8. github-actions

   closed this as completedon Jan 15, 2021