tsfn: implement TypedThreadSafeFunction #742

KevinEady · 2020-06-08T14:30:36Z

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:

…i-gcc-4

KevinEady · 2020-06-08T14:44:46Z

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 · 2020-06-12T16:06:39Z

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?

It should actually check for empty jsCallback

gabrielschulhof · 2020-11-19T03:39:57Z

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 · 2020-11-20T16:43:02Z

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

KevinEady · 2020-11-20T17:17:53Z

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

mhdawson · 2020-11-23T16:01:28Z

@KevinEady, landed #832

mhdawson · 2020-11-23T16:02:14Z

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

…i-gcc-4

KevinEady · 2020-11-24T17:37:54Z

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 · 2020-11-25T12:54:31Z

@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 · 2020-11-26T23:15:14Z

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 · 2020-11-27T15:27:46Z

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 · 2020-11-30T14:20:15Z

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 · 2020-11-30T14:21:20Z

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 · 2020-11-30T14:39:37Z

@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 · 2020-11-30T16:36:50Z

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 · 2020-11-30T22:40:44Z

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 · 2020-11-30T22:44:14Z

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 · 2020-12-01T20:58:11Z

@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 · 2020-12-01T21:27:54Z

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

@mhdawson I have no problems with that.

gabrielschulhof · 2020-12-04T01:54:14Z

@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 · 2020-12-07T19:16:55Z

CI on resulting master was green:

48e6b58#commitcomment-44890203

KevinEady added 12 commits March 22, 2020 19:34

tsfn: implement ThreadSafeFunctionEx<ContextType>

78196e0

fix unused parameter errors

90ebd80

wip

fc339c4

wip

fd530b2

make CallJS template parameter

561b968

statically check CallJs; add no-Finalizer overload

630d88b

add tsfn test with tsfn cb function call

9ff352c

Merge remote-tracking branch 'upstream/master' into contexted-tsfn-api

6aeecd4

test: Initial commit of TSFNEx threadsafe test

2da3023

test: Modify TSFNEx threadsafe to use TSFNEx

91e8859

src,test: Add SIGTRAP to TSFNEx::CallJS

692fbe1

Merge remote-tracking branch 'upstream/master' into contexted-tsfn-ap…

0d26703

…i-gcc-4

KevinEady requested review from gabrielschulhof and mhdawson June 8, 2020 14:30

KevinEady requested a review from legendecas June 8, 2020 14:45

KevinEady changed the title ~~Contexted tsfn api gcc 4~~ tsfn: implement ThreadSafeFunctionEx Jun 8, 2020

KevinEady added 12 commits June 12, 2020 22:50

test: fix TSFNEx tests

89181da

test: add barebones new tsfn test for use in docs

8fcdb4d

implement optional function callback

cc8de12

clean up optional callback implementation

6706f96

test: wip with tsfnex tests

89d2dea

test: napi v4,v5 all tests pass

d2bcc03

test: consolidate duplicated code

6b7a7d0

test: basic example, standardize identifier names

7467a3f

test: refactor the the 'empty' tsfnex test

a01c3c8

It should actually check for empty jsCallback

basic multi-threading

b0e7817

test: wip with example

44adeea

test: wip with example test

c20685b

KevinEady added 2 commits November 24, 2020 18:12

Rename to TypedThreadSafeFunction

c24c455

Merge remote-tracking branch 'upstream/master' into contexted-tsfn-ap…

559ad8c

…i-gcc-4

KevinEady force-pushed the contexted-tsfn-api-gcc-4 branch from be32dc8 to 559ad8c Compare November 24, 2020 17:13

Apply formatting changes

5e5b9ce

KevinEady changed the title ~~tsfn: implement ThreadSafeFunctionEx~~ tsfn: implement TypedThreadSafeFunction Nov 24, 2020

KevinEady requested a review from legendecas November 24, 2020 17:40

legendecas approved these changes Dec 4, 2020

View reviewed changes

gabrielschulhof merged commit 48e6b58 into nodejs:master Dec 7, 2020

This was referenced Dec 15, 2020

Make callback optional in ThreadSafeFunction::New #859

Closed

Semantical difference between Napi::ThreadSafeFunction and napi_threadsafe_function #594

Closed

mhdawson mentioned this pull request Dec 16, 2020

Fundamental issues with ThreadSafeFunction #665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsfn: implement TypedThreadSafeFunction #742

tsfn: implement TypedThreadSafeFunction #742

KevinEady commented Jun 8, 2020 •

edited

Loading

KevinEady commented Jun 8, 2020

KevinEady commented Jun 12, 2020 •

edited

Loading

gabrielschulhof commented Nov 19, 2020

mhdawson commented Nov 20, 2020 •

edited

Loading

KevinEady commented Nov 20, 2020

mhdawson commented Nov 23, 2020

mhdawson commented Nov 23, 2020

KevinEady commented Nov 24, 2020

legendecas commented Nov 25, 2020

mhdawson commented Nov 26, 2020

mhdawson commented Nov 27, 2020

mhdawson commented Nov 30, 2020

mhdawson commented Nov 30, 2020

KevinEady commented Nov 30, 2020

KevinEady commented Nov 30, 2020

mhdawson commented Nov 30, 2020

mhdawson commented Nov 30, 2020

mhdawson commented Dec 1, 2020

KevinEady commented Dec 1, 2020

gabrielschulhof commented Dec 4, 2020

legendecas left a comment

gabrielschulhof commented Dec 7, 2020

tsfn: implement TypedThreadSafeFunction #742

tsfn: implement TypedThreadSafeFunction #742

Conversation

KevinEady commented Jun 8, 2020 • edited Loading

KevinEady commented Jun 8, 2020

KevinEady commented Jun 12, 2020 • edited Loading

gabrielschulhof commented Nov 19, 2020

mhdawson commented Nov 20, 2020 • edited Loading

KevinEady commented Nov 20, 2020

mhdawson commented Nov 23, 2020

mhdawson commented Nov 23, 2020

KevinEady commented Nov 24, 2020

legendecas commented Nov 25, 2020

mhdawson commented Nov 26, 2020

mhdawson commented Nov 27, 2020

mhdawson commented Nov 30, 2020

mhdawson commented Nov 30, 2020

KevinEady commented Nov 30, 2020

KevinEady commented Nov 30, 2020

mhdawson commented Nov 30, 2020

mhdawson commented Nov 30, 2020

mhdawson commented Dec 1, 2020

KevinEady commented Dec 1, 2020

gabrielschulhof commented Dec 4, 2020

legendecas left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Dec 7, 2020

KevinEady commented Jun 8, 2020 •

edited

Loading

KevinEady commented Jun 12, 2020 •

edited

Loading

mhdawson commented Nov 20, 2020 •

edited

Loading