tsfn: Implement copy constructor #546

KevinEady · 2019-09-18T21:32:47Z

Removes the unique_ptr from ThreadSafeFunction, thereby allowing
copies.

Removes the `unique_ptr` from `ThreadSafeFunction`, thereby allowing copies. Ref: nodejs#524

legendecas

Should copied ThreadSafeFunction be acquired automatically and requires a release?

KevinEady · 2019-09-19T08:58:26Z

So... IMO I don't think it should automatically acquire/release. I believe it adds an assumption that each copy must perform an Acquire, which I think is not true. In fact, I think it would actually cause problems in certain implementations.

Also, if Acquire() returns napi_closing, I would not consider this a fatal error so we shouldn't throw in the copy ctor. How would we handle this? Storing the status on the newly constructed object for the developer to check after copy, eg. in the thread entry point? I dunno, seems clunky...

gabrielschulhof · 2019-09-23T19:08:39Z

@legendecas IINM passing to another thread may not be the only reason for copying a Napi::ThreadSafeFunction. For example, if you used it as a key in a hash table ...

gabrielschulhof · 2019-09-23T19:09:28Z

Also IINM it should be easy to subclass Napi::ThreadSafeFunction and add the automatic Acquire()/Release() for those that need it.

KevinEady · 2019-10-02T19:17:19Z

So I have no idea about the ambiguous function calls for New as mentioned in #544 (comment) but I don't think it should block this. If so, I believe this is ready.

mhdawson · 2019-10-03T20:24:14Z

I agree that the problem related to ambiguous function calls is independent and could be addressed in a follow on PR.

mhdawson · 2019-10-03T20:54:14Z

test/threadsafe_function/threadsafe_function_sum.cc

+  TestData* testData = static_cast<TestData*>(info.Data());
+  ThreadSafeFunction tsfn = testData->tsfn;
+  int threadId = testData->threads.size();
+  testData->threads.push_back( thread(entryAcquire, tsfn, threadId) );


I wonder if it would be useful to highlight with a comment that this is where the copy constructor is being tested. Since the main change is to add the copy constructor it would be good to make it obvious in the test that it is being used.

Added comment

mhdawson

LGTM

KevinEady · 2019-10-23T10:19:56Z

Hey @mhdawson, can we get this landed so I can continue work on Ref/Unref as well as the incorrect [Non]BlockingCall with ThreadSafeFunction(napi_threadsafe_function) constructor

mhdawson · 2019-10-23T14:25:47Z

I'm in the process of landing. It did not seem to compile with master and the older 4.8.5 compiler. That may just be because master need gcc 6, but I want to run a CI on 8.x before landing.

Unfortunately, the CI job is having some issues getting the Node binaries so I'll have to figure that out first.

mhdawson · 2019-10-23T18:15:18Z

This seems to fail to build on 8.x with the 4.8.5 compiler level. You can see the compile failures her: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/nodes=ubuntu1604-64/822/console

mhdawson · 2019-10-23T18:16:21Z

@KevinEady can you take a look:

../threadsafe_function/threadsafe_function_sum.cc: In function ‘Napi::Value {anonymous}::TestWithTSFN(const Napi::CallbackInfo&)’:
../threadsafe_function/threadsafe_function_sum.cc:51:4: error: no matching function for call to ‘{anonymous}::TestData::TestData(<brace-enclosed initializer list>)’
   });
    ^
../threadsafe_function/threadsafe_function_sum.cc:12:8: note: candidate: {anonymous}::TestData::TestData(const {anonymous}::TestData&)
 struct TestData {
        ^
../threadsafe_function/threadsafe_function_sum.cc:12:8: note:   no known conversion for argument 1 from ‘<brace-enclosed initializer list>’ to ‘const {anonymous}::TestData&’

KevinEady · 2019-10-24T19:26:30Z

Hey team,

So I was actually testing this on Windows, and it was failing because defining a move operator implicitly deletes the assignment operator... And, if you think about it, the ThreadSafeFunction wrapper only has for its member data this napi_threadsafe_function which behaves as a pointer: we can use the default compiler-generated functions anyways, right?

test\threadsafe_function\threadsafe_function_sum.cc(90): error C2280: 'Napi::ThreadSafeFunction &Napi::ThreadSafeFunction::operator =(const Napi::ThreadSafeFunction &)': attempting to reference a deleted function [test\build\binding_noexcept.vcxproj]
  napi.h(2088): note: compiler has generated 'Napi::ThreadSafeFunction::operator =' here
  napi.h(2088): note: 'Napi::ThreadSafeFunction &Napi::ThreadSafeFunction::operator =(const Napi::ThreadSafeFunction &)': function was implicitly deleted because 'Napi::ThreadSafeFunction' has a user-defined move constructor (compiling source file ..\threadsafe_function\threadsafe_function_sum.cc)

I did not think it was worthwhile to explicitly create these constructors, nor state them as default in napi.h , so I just removed them. Since this is structural change, I've dismissed your review @mhdawson

mhdawson · 2019-10-24T22:06:17Z

CI job to validate builds on Node 8.x: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/854/

mhdawson · 2019-10-24T22:09:11Z

@KevinEady seems to be failures on osx and aix: : https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/854/

KevinEady · 2019-10-25T09:10:34Z

Hi @mhdawson ,

Fixed some issues in the tests that I know were problematic, but this one is really stumping me..

https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/nodes=osx1011/855/console :

/Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/future:1170:1: error: 'future<Napi::ThreadSafeFunction>' is unavailable: introduced in macOS 10.8
future<_Rp>::get()

I don't know, just looking at the node name os osx1011 ... but is this build targeting osx 10.8 ...?

KevinEady · 2019-10-29T20:48:41Z

Hi @mhdawson / @gabrielschulhof ,

So I re-wrote my tests to use std::condition_variable and std::mutex instead of promises/futures. Let me know if this is suitable now.

legendecas · 2019-10-30T14:17:45Z

So I re-wrote my tests to use std::condition_variable and std::mutex instead of promises/futures. Let me know if this is suitable now.

I noticed there are still usages on promises/futures in test/threadsafe_function/threadsafe_function_sum.cc. Are they intended?

KevinEady · 2019-10-30T14:42:53Z

Hi @legendecas ,

I noticed there are still usages on promises/futures in test/threadsafe_function/threadsafe_function_sum.cc. Are they intended?

So I noticed I did not remove the include, which I now replaced with the correct ones for use with condition_variable/mutex.

However, the promise / deferred you see are Napi::Promises, not std library. They are used in the test to ensure proper synchronization. Does this address your concern?

mhdawson · 2019-10-30T22:29:34Z

CI run to see if it works on 8.x https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/906/

mhdawson

LGTM

mhdawson · 2019-10-31T01:31:21Z

@legendecas let us know if the latest changes address your concerns and then approve if appropriate. Will wait to hear from you before landing.

legendecas

LGTM overall with one nit...

legendecas · 2019-10-31T02:26:31Z

napi-inl.h

 }

 inline ThreadSafeFunction::ConvertibleContext
 ThreadSafeFunction::GetContext() const {
  void* context;
-  napi_get_threadsafe_function_context(*_tsfn, &context);
+  napi_get_threadsafe_function_context(_tsfn, &context);


Abort if failed here?

Since the change doesn't change the behavior here, we could address this issue in another PR. Opened issue here #581.

@legendecas I agree, can you open an issue to make sure we fix it later?

@mhdawson the issue is above, and PR for fix (which is also conflicting with this PR) is #583

@KevinEady - oops should have read more carefullly. Thanks.

mhdawson · 2019-11-01T20:35:27Z

Landed as 2e71842

* tsfn: Implement copy constructor Refs: nodejs/node-addon-api#524 PR-URL: nodejs/node-addon-api#546 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

tsfn: Implement copy constructor

1b42bfc

Removes the `unique_ptr` from `ThreadSafeFunction`, thereby allowing copies. Ref: nodejs#524

legendecas reviewed Sep 19, 2019

View reviewed changes

KevinEady added 2 commits October 2, 2019 20:03

Merge remote-tracking branch 'upstream/master' into tsfn-copy-ctor

e9f38dc

change FinalizerCallback to static method

e10e683

mhdawson reviewed Oct 3, 2019

View reviewed changes

tsfn: add copy ctor comments to test

dfa9e5f

This was referenced Oct 9, 2019

tsfn: Add wrappers for Ref and Unref #561

Closed

ThreadSafeFunction(napi_threadsafe_function) works badly with BlockingCall() #556

Closed

mhdawson approved these changes Oct 9, 2019

View reviewed changes

NickNaso mentioned this pull request Oct 22, 2019

Next release 2.0.0 #570

Closed

15 tasks

KevinEady added 4 commits October 24, 2019 20:16

merge master, resolve conflicts

a033340

tsfn: remove unnecessary constructors, operators

815c1fe

test: change tsfn test data construction

3219515

test: fix tsfn tests by napi version

0e9d4ae

KevinEady requested a review from mhdawson October 24, 2019 19:25

test: use explicit std qualifiers in tsfn test

cba39bf

tsfn: use mutex, cv for tests vs promise

5b83cf2

Merge branch 'master' into tsfn-copy-ctor

5011023

KevinEady added 2 commits October 30, 2019 15:32

test: remove unnecessary include

4b3d134

test: add std includes

bcdc46c

mhdawson approved these changes Oct 31, 2019

View reviewed changes

legendecas approved these changes Oct 31, 2019

View reviewed changes

legendecas mentioned this pull request Oct 31, 2019

Error was ignored if ThreadSafeFunction::GetContext() failed #581

Closed

mhdawson merged commit 2e71842 into nodejs:master Nov 1, 2019

KevinEady mentioned this pull request Nov 6, 2019

Destroying Napi::ThreadSafeFunction instances causing segfault, but Linux only? #592

Closed

KevinEady mentioned this pull request Dec 16, 2019

ThreadSafeFunction memory violation in ThreadSafeFinalize #632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsfn: Implement copy constructor #546

tsfn: Implement copy constructor #546

KevinEady commented Sep 18, 2019

legendecas left a comment

KevinEady commented Sep 19, 2019

gabrielschulhof commented Sep 23, 2019

gabrielschulhof commented Sep 23, 2019

KevinEady commented Oct 2, 2019

mhdawson commented Oct 3, 2019

mhdawson Oct 3, 2019

KevinEady Oct 9, 2019

mhdawson left a comment

KevinEady commented Oct 23, 2019

mhdawson commented Oct 23, 2019

mhdawson commented Oct 23, 2019

mhdawson commented Oct 23, 2019

KevinEady commented Oct 24, 2019

mhdawson commented Oct 24, 2019

mhdawson commented Oct 24, 2019

KevinEady commented Oct 25, 2019

KevinEady commented Oct 29, 2019

legendecas commented Oct 30, 2019

KevinEady commented Oct 30, 2019

mhdawson commented Oct 30, 2019

mhdawson left a comment

mhdawson commented Oct 31, 2019

legendecas left a comment

legendecas Oct 31, 2019

legendecas Oct 31, 2019

mhdawson Nov 1, 2019

KevinEady Nov 1, 2019

mhdawson Nov 1, 2019

mhdawson commented Nov 1, 2019

tsfn: Implement copy constructor #546

tsfn: Implement copy constructor #546

Conversation

KevinEady commented Sep 18, 2019

legendecas left a comment

Choose a reason for hiding this comment

KevinEady commented Sep 19, 2019

gabrielschulhof commented Sep 23, 2019

gabrielschulhof commented Sep 23, 2019

KevinEady commented Oct 2, 2019

mhdawson commented Oct 3, 2019

mhdawson Oct 3, 2019

Choose a reason for hiding this comment

KevinEady Oct 9, 2019

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

KevinEady commented Oct 23, 2019

mhdawson commented Oct 23, 2019

mhdawson commented Oct 23, 2019

mhdawson commented Oct 23, 2019

KevinEady commented Oct 24, 2019

mhdawson commented Oct 24, 2019

mhdawson commented Oct 24, 2019

KevinEady commented Oct 25, 2019

KevinEady commented Oct 29, 2019

legendecas commented Oct 30, 2019

KevinEady commented Oct 30, 2019

mhdawson commented Oct 30, 2019

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Oct 31, 2019

legendecas left a comment

Choose a reason for hiding this comment

legendecas Oct 31, 2019

Choose a reason for hiding this comment

legendecas Oct 31, 2019

Choose a reason for hiding this comment

mhdawson Nov 1, 2019

Choose a reason for hiding this comment

KevinEady Nov 1, 2019

Choose a reason for hiding this comment

mhdawson Nov 1, 2019

Choose a reason for hiding this comment

mhdawson commented Nov 1, 2019