tsfn: implement ThreadSafeFunctionEx for GCC 5+ #687

KevinEady · 2020-03-22T18:43:03Z

This PR backs the existing ThreadSafeFunction class with the new ThreadSafeFunctionEx class. However, there are compilation issues on gcc-4.8 used by the CI. node v10 has a minimum build requirement of gcc-4.9.

We discussed somewhere (possibly an N-API meeting) that we can have this PR blocked until node 10 is decommissioned, so we can update our CI's gcc and possibly incorporate these changes.

legendecas · 2020-03-23T02:22:00Z

napi-inl.h

+                                                  ContextType* context,
+                                                  Finalizer finalizeCallback,
+                                                  FinalizerDataType* data,
+                                                  napi_threadsafe_function_call_js call_js_cb) {


I believe it is worth to type the napi_threadsafe_function_call_js with ContextType and DataType.

@legendecas I was thinking that too, however looking at #594 (comment) , there were complaints that the "wrapping" for CallJS was a performance hit... and I can't think of a way to type napi_threadsafe_function_call_js without making a wrapper.

Furthermore, I don't think DataType can be typed without a wrapper, because it's "declared" in ::New but "used" in ::[Non]BlockingCall, and not sure how to link the two together. The way it works in the existing TSFN is that the napi_threadsafe_function_call_js takes this CallbackWrapper data which is appropriately typed. Without a wrapper, not sure how to accomplish...

KevinEady · 2020-03-24T09:13:35Z

Hi @mhdawson / @gabrielschulhof / @legendecas,

In yesterday's meeting we discussed how I removed the multiple overloads for the New method and only kept one that has all parameters. Looking at #544, the last two comments highlight two additional concerns regarding the optional parameter overloads. I believe those are valid, and that's why we I feel we should not have all the possible overloads. This leaves it up to the developer to pass appropriate values. It would also inherently keep up with changing N-API functionality (eg. the new optional Function parameter).

On the contrary, if we did have the multiple overloads, we could N-API version target them via various preprocessor directives, eg. having New with no Function if NAPI > x.

I do think we should just stick with one New method.

Thoughts?

legendecas · 2020-03-26T17:07:22Z

@KevinEady I submitted another issue #690 to propose an alternative for the optional Function of New parameter.

So with Maybe boxes, I'd believe we can lean on the one single New.

KevinEady · 2020-03-30T09:58:34Z

So @gabrielschulhof / @legendecas / @mhdawson ,

I was working on this over the weekend, and regarding the CallJS wrapper...

To recall, we wanted to wrap it so the New function's call_js_cb

node-addon-api/napi.h

Lines 2045 to 2054 in 90ebd80

    
           static ThreadSafeFunctionEx<ContextType> New(napi_env env, 
        
                                         const Function& callback, 
        
                                         const Object& resource, 
        
                                         ResourceString resourceName, 
        
                                         size_t maxQueueSize, 
        
                                         size_t initialThreadCount, 
        
                                         ContextType* context, 
        
                                         Finalizer finalizeCallback, 
        
                                         FinalizerDataType* data, 
        
                                         napi_threadsafe_function_call_js call_js_cb);

eg. callback

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

Lines 82 to 85 in 90ebd80

    
           [](napi_env, napi_value, void *context, void *data) { // call_js_cb 
        
             std::unique_ptr<CallJsData> callData(static_cast<CallJsData*>(data)); 
        
             callData->resolve(static_cast<TSFNContext*>(context)); 
        
           });

would have signature

void(Napi::Env, Napi::Function, ContextType *context, void *data)

However, if we do wrap the callback given to napi_create_threadsafe_function, you would not be able to get the wrapper ThreadSafeFunctionEx<Context>'s underlying napi_threadsafe_function and use it directly with napi_call_threadsafe_function, eg. this behavior would not work:

auto tsfn = Napi::ThreadSafeFunctionEx<Context>::New(env, ....);
napi_call_threadsafe_function(tsfn, ...)

I imagine this is currently an issue in the existing ThreadSafeFunction implementation as well.

FWIW, there was a related problem #556 due to wrapping

KevinEady · 2020-04-07T17:48:20Z

Hey @mhdawson / @legendecas / @gabrielschulhof ,

I was able to template the callback parameter as Gabe suggested (woohoo!), and I think the API feels alright... I've included two test cases that show how to use the API:

threadsafe_function_ex/context.cc - showcases a use of ThreadSafeFunctionEx with all templated parameters (context, data type, call js)
threadsafe_function_ex/simple.cc - a very basic tsfn that gives the bare-minimum

I had to add an overload for New that took no finalizer but that is the single overload, as the finalizer data is defaulted to nullptr.

What do you guys think of this implementation?

gabrielschulhof · 2020-04-16T23:08:41Z

@KevinEady could you please rebase this so I can try a CI run? As it stands, the CI is affected by #695 and our CI doesn't automatically rebase for testing like core's CI does.

KevinEady · 2020-04-17T12:57:52Z

Hi @gabrielschulhof , I've updated with master.

Also, I've added another test to showcase intended api some more -- threadsafe_function_ex/call.cc with no context nor finalizer. /cc @mhdawson @legendecas .

gabrielschulhof

The implementation looks good, though AFAICT none of the tests are actually multithreaded. It would be good to have the Napi::ThreadSafeFunctions tests reproduced here to ensure that the class deals correctly with threads.

mhdawson · 2020-04-24T15:32:47Z

napi-inl.h

+template <typename ContextType, typename DataType, typename CallJs, CallJs call>
+typename std::enable_if<call != nullptr>::type
+static inline CallJsWrapper(napi_env env, napi_value jsCallback, void *context, void *data) {
+  call(env, Function(env, jsCallback), static_cast<ContextType *>(context),


Does this need the if (jsCallback != nullptr) check like the version below or do we expect the CallJs

Hi @mhdawson ,

Your comment might have got cut off / incomplete but I think I got you...

I do not think we need the jsCallback != nullptr check like below. The call will go into the user-defined CallJs call with an empty function, which I believe is the normal behavior of using the underlying napi tsfn calls with no callback Function but with a CallJs.

mhdawson · 2020-04-24T15:33:04Z

I assume we need some additions to the docs as well?

KevinEady · 2020-04-26T14:03:43Z

The implementation looks good, though AFAICT none of the tests are actually multithreaded. It would be good to have the Napi::ThreadSafeFunctions tests reproduced here to ensure that the class deals correctly with threads.

Hi @gabrielschulhof / @mhdawson / @legendecas

Since the ThreadSafeFunctionEx class can be considered the "base feature" class, I was able to make the existing ThreadSafeFunction class inherit from Ex, thus the existing tests inherently go through Ex. Does this satisfy? Please take a look... None of the public members were changed so I do not think this would affect any backwards compatibility

Well... Going through the different CI errors, messing with templates is not as easy as I imagined.

KevinEady · 2020-04-26T15:57:33Z

Hi team,

Going through the errors seen here https://travis-ci.com/github/nodejs/node-addon-api/jobs/323508516#L743 ...

I noticed the node-addon-api CI uses gcc 4.8.4 but the minimum supported gcc version for building node is gcc 6. Aaaand of course, when I test this PR on a gcc 6.1 docker image, it runs fine.

Do we need to support gcc 4? Let's discuss in the next call.

richardlau · 2020-04-27T16:23:16Z

Do we need to support gcc 4? Let's discuss in the next call.

If you ever want this to land on 10.x then, yes:
https://github.com/nodejs/node/blob/v10.x/BUILDING.md#unix

mhdawson · 2022-01-21T17:16:30Z

We did not discuss in the Node-api team meeting as we ran out of time, but I think since 10.x is officially not supported it can be revisited. See just above https://github.com/nodejs/node-addon-api#setup for where we have documented that.

KevinEady added the work in progress (WIP) label Mar 22, 2020

KevinEady mentioned this pull request Mar 22, 2020

Fundamental issues with ThreadSafeFunction #665

Closed

KevinEady mentioned this pull request Mar 22, 2020

[WIP] tsfn: Add context to [Non]BlockingCall callback #676

Closed

2 tasks

legendecas reviewed Mar 23, 2020

View changes

KevinEady changed the title ~~tsfn: implement ThreadSafeFunctionEx<ContextType>~~ tsfn: implement ThreadSafeFunctionEx Apr 8, 2020

gabrielschulhof reviewed Apr 20, 2020

View changes

mhdawson reviewed Apr 24, 2020

View changes

gabrielschulhof mentioned this pull request Apr 27, 2020

Upgrade gcc on the CI #710

Closed

KevinEady changed the title ~~tsfn: implement ThreadSafeFunctionEx~~ tsfn: implement ThreadSafeFunctionEx for GCC 5+ Jun 8, 2020

KevinEady mentioned this pull request Jun 15, 2020

tsfn: implement TypedThreadSafeFunction #742

Merged

2 tasks

KevinEady added 2 commits Aug 18, 2020

checkout contexted-tsfn-api-gcc-4 -- napi.h napi-inl.h

81a8f9b

src: Implement TSFN backed by TSFNEx

c7ec57b

KevinEady force-pushed the contexted-tsfn-api branch from 6a7b764 to c7ec57b Compare Aug 19, 2020

KevinEady added blocked and removed work in progress (WIP) labels Aug 19, 2020

Base automatically changed from master to main Jan 26, 2021

github-actions bot added the stale label Dec 21, 2021

github-actions bot closed this Jan 20, 2022

mhdawson reopened this Jan 20, 2022

mhdawson removed the stale label Jan 20, 2022

nodejs / node-addon-api Public

tsfn: implement ThreadSafeFunctionEx for GCC 5+ #687

tsfn: implement ThreadSafeFunctionEx for GCC 5+ #687

KevinEady commented Mar 22, 2020 •

edited

legendecas Mar 23, 2020

KevinEady Mar 23, 2020

KevinEady commented Mar 24, 2020

legendecas commented Mar 26, 2020

KevinEady commented Mar 30, 2020

KevinEady commented Apr 7, 2020 •

edited

gabrielschulhof commented Apr 16, 2020

KevinEady commented Apr 17, 2020

gabrielschulhof left a comment

mhdawson Apr 24, 2020

KevinEady Apr 26, 2020

mhdawson commented Apr 24, 2020

KevinEady commented Apr 26, 2020

KevinEady commented Apr 26, 2020

richardlau commented Apr 27, 2020

mhdawson commented Jan 21, 2022

Mar	APR	May
	05
2021	2022	2023

nodejs / node-addon-api Public

tsfn: implement ThreadSafeFunctionEx for GCC 5+ #687

Are you sure you want to change the base?

tsfn: implement ThreadSafeFunctionEx for GCC 5+ #687

Conversation

KevinEady commented Mar 22, 2020 • edited

legendecas Mar 23, 2020

Choose a reason for hiding this comment

KevinEady Mar 23, 2020

Choose a reason for hiding this comment

KevinEady commented Mar 24, 2020

legendecas commented Mar 26, 2020

KevinEady commented Mar 30, 2020

KevinEady commented Apr 7, 2020 • edited

gabrielschulhof commented Apr 16, 2020

KevinEady commented Apr 17, 2020

gabrielschulhof left a comment

mhdawson Apr 24, 2020

Choose a reason for hiding this comment

KevinEady Apr 26, 2020

Choose a reason for hiding this comment

mhdawson commented Apr 24, 2020

KevinEady commented Apr 26, 2020

KevinEady commented Apr 26, 2020

richardlau commented Apr 27, 2020

mhdawson commented Jan 21, 2022

KevinEady commented Mar 22, 2020 •

edited

KevinEady commented Apr 7, 2020 •

edited