tsfn: implement TypedThreadSafeFunction

[KevinEady]

Implements a templated TypedThreadSafeFunction<ContextType, DataType, Callback> class. The class also adds static, compile-time support for the optional Function argument on N-API 5+ using std::nullptr_t (as well as completely missing Function argument) on N-API 5+.

TODO:

• Documentation
• (optional) see if the existing ThreadSafeFunction class can utilize this one, as done in previous POC tsfn: implement ThreadSafeFunctionEx for GCC 5+ #687 (comment)
  • Completed in tsfn: implement TypedThreadSafeFunction #742 (comment)

Fixes:

• Fundamental issues with ThreadSafeFunction #665
• Context parameter on thread safe function callback #653
• Semantical difference between Napi::ThreadSafeFunction and napi_threadsafe_function #594

[KevinEady]

Hey @gabrielschulhof , @mhdawson ... Soooo I am a little stuck now. I have no idea why this is happening, but it seems that the TSFNEx::CallJs wrapper (which, in turn, calls the user-defined static CallJs function) is getting called by Node with an empty napi_env.

I did as you suggested, and migrated the old "multi-threaded" TSFN test to this new TSFNEx methodology. The changes to do so seem minimal, so I'm not sure why this is erroring.

I've added this signal(SIGTRAP) call, and you can see it is indeed erroring there the CI build.

This is what I've got in my debugger:

and here's the stack...

__pthread_kill (@__pthread_kill:6)
pthread_kill (@pthread_kill:109)
raise (@raise:12)
Napi::ThreadSafeFunctionEx<ThreadSafeFunctionInfo, int, &(TSFNCallJS(Napi::Env, Napi::Function, ThreadSafeFunctionInfo*, int*))>::CallJsInternal(napi_env__*, napi_value__*, void*, void*) (/Users/kevineady/Documents/Projects/node-addon-api/napi-inl.h:4477)
void node::Environment::CloseHandle<uv_handle_s, v8impl::(anonymous namespace)::ThreadSafeFunction::CloseHandlesAndMaybeDelete(bool)::'lambda'(uv_handle_s*)::operator()(uv_handle_s*) const::'lambda'(uv_handle_s*)>(uv_handle_s*, v8impl::(anonymous namespace)::ThreadSafeFunction::CloseHandlesAndMaybeDelete(bool)::'lambda'(uv_handle_s*)::operator()(uv_handle_s*) const::'lambda'(uv_handle_s*))::'lambda'(uv_handle_s*)::__invoke(uv_handle_s*) (@void node::Environment::CloseHandle<uv_handle_s, v8impl::(anonymous namespace)::ThreadSafeFunction::CloseHandlesAndMaybeDelete(bool)::'lambda'(uv_handle_s*)::operator()(uv_handle_s*) const::'lambda'(uv_handle_s*)>(uv_handle_s*, v8impl::(anonymous namespace)::ThreadSafeFunction::CloseHandlesAndMaybeDelete(bool)::'lambda'(uv_handle_s*)::operator()(uv_handle_s*) const::'lambda'(uv_handle_s*))::'lambda'(uv_handle_s*)::__invoke(uv_handle_s*):71)
uv_run (@uv_run:142)
node::NodeMainInstance::Run() (@node::NodeMainInstance::Run():100)
node::Start(int, char**) (@node::Start(int, char**):61)
start (@start:4)

Hopefully we can discuss in today's call.

[KevinEady]

Hi @gabrielschulhof ,

I took into a look into what you suggested, that maybe all threads have called Release() prior to queue being completed.

I pinpointed the failure to this specific portion:

// Start the thread in blocking mode, and assert that it could not finish.// Quit early by aborting..then(()=>testWithJSMarshaller({threadStarter: 'startThread',quitAfter: 1,maxQueueSize: binding.threadsafe_function_ex_threadsafe.MAX_QUEUE_SIZE,abort: true})).then((result)=>assert.strictEqual(result.indexOf(0),-1))

... and when my CallJS is triggered, the environment and callback are null, but my context and data are still valid:

So I think you are completely right: this test has the TSFN abort, so I suppose there are still items on the queue when an abort happens.

So, am I correct that:

• Inside your CallJS, checking that env == nullptr signifies that the thread count on the TSFN is 0, ie. all have called Release / Abort / Call (and received napi_closing)
• However, if you want to handle the case that 'tsfn is aborted' differently inside your CallJS, this must be managed by user, as napi does not provide a threadsafe way to get the tsfn state.
  • ... but I suppose it wouldn't make sense to handle the logic that "thread is aborted" in the CallJS callback, as you would probably handle it on the caller side, when receiving napi_closing fromtsfn.Call

EDIT: I ask for clarification, because I want to put this in the documentation. I don't think it's inherently clear that your TSFN's "environment" is cleaned up on the last Release. I think it's clear that the finalizer will run on the last release, but I don't think it's clear that your tsfn can be finalized with items still on the queue. Possible workflow: last Release() is called -> queue is executed -> tsfn finalizer ran... but I suppose it is done to prevent the CallJS callback from potentially adding more items to the tsfn.

Do you know of any place where we have documentation about the "CallJS can be ran with empty napi env/function after the TSFN has been finalized, for you to process the remaining items on the queue" or whatever?

[gabrielschulhof]

CI:

Version	Job	Status
v16.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/3101/
v15.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/3102/
v14.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/3103/
v12.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/3104/
v10.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/3105/

[mhdawson]

From discussion in the meeting we came up with another suggestion which was TypedThreadSafeFunction

[KevinEady]

When #832 lands, I will rebase, rename, and push.

[mhdawson]

@KevinEady, landed #832

[mhdawson]

@KevinEady we suggested TypedThreadSafeFunction as a possibility but wanted your input on whether you thought that make sense or not.

[KevinEady]

Hi @mhdawson , I have rebased + renamed to TypedThreadSafeFunction. I noticed the CI on push only checks for style changes. Are you doing changes on this @legendecas?

@gabrielschulhof / @mhdawson, can you run the CI?

[legendecas]

@KevinEady I'm not sure what's going on with TravisCI. We do have .travis.yml unchanged since 8 days ago on the master branch.

I checked the travis permission and found out that Node.js github org doesn't grant any public write access to travis. Not sure if it is a recent change.

[mhdawson]

CI runs:

10.x - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3149/
master - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3150/

[mhdawson]

12.x - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3157/
14.x - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3156/
15.x - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3158/

[mhdawson]

12.x had a failure so re-run: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3174/

[mhdawson]

This was the failure on 12.x on Mac 15

Running test'threadsafe_function/threadsafe_function_sum'
Build timed out (after 60 minutes). Marking the build as failed.

[KevinEady]

@mhdawson That is... odd. That test isn't part of this PR, and there were no changes made to the existing TSFN implementation. FWIW, the previous 12.x run passed, which was manual test for HandleScope fix with old ThreadSafeFunctionEx name.

[KevinEady]

Looks like https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3174/ passed... do you think there is some issue with the original tsfn test?

[mhdawson]

Seems like the re-run I launched was on 16.x. Another one on 12.x this time: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/3175/

@KevinEady my first guess is an existing problem with the test unless somehow the PR refactored code which the base ThreadSafeFunction uses?

[mhdawson]

Looks like this simply adds to napi-inl.h so most likely an existing flaky issue with the test. As long as the new 12.x run I launched passes I think we can land this PR.

@KevinEady if you are ok to land then I think we should be good to go.

[mhdawson]

@legendecas are you ok with this landing now? Just want to make sure as it shows requested changes, but I think that was just to close on the name change.

[KevinEady]

@KevinEady if you are ok to land then I think we should be good to go.

@mhdawson I have no problems with that.

[gabrielschulhof]

@legendecas I think you can remove the -1, because the class was renamed.

[legendecas]

Sorry for the delay. This is a great work! LGTM

[gabrielschulhof]

CI on resulting master was green:

48e6b58#commitcomment-44890203