Implement ThreadSafeFunction class #442

romandev · 2019-02-05T15:02:01Z

This PR is implementing ThreadSafeFunction class wraps
napi_threadsafe_function features.

FYI, the test files that included in this PR have come from Node.js
repo[1]. They've been rewritten based on C++ and node-addon-api.

Fixes #312.

[1] https://github.com/nodejs/node/tree/master/test/node-api/test_threadsafe_function

mhdawson · 2019-02-06T21:10:10Z

@gabrielschulhof would be good if you took a look at this one.

gabrielschulhof

This PR is still WIP AFAICT.

napi.h

test/threadsafe_function/threadsafe_function.cc

test/threadsafe_function/threadsafe_function.js

test/threadsafe_function/threadsafe_function.cc

romandev · 2019-03-06T15:56:20Z

@gabrielschulhof, @mhdawson PTAL
(If you don't mind, I'd like to push docs update into separated PR because this PR is enough large)

KevinEady

Should ThreadSafeFunction::New have multiple overloaded methods? According to the docs, async_resource, thread_finalize_data/thread_finalize_cb, and context are optional, so I would think ThreadSafeFunction::New would have overloads allowing me to bypass them.

napi-inl.h

mhdawson · 2019-03-12T20:23:12Z

@romandev I'm ok with the docs being in a different PR, but I'd prefer if the two were landed together as opposed to landing one without the other. That might make it difficult to do a release as I think we want to have both in place before having the functionality go out in a release.

KevinEady · 2019-03-15T16:15:20Z

Is there anything I can do to help with this PR? Maybe docs or something? I have taken a look, and have been successfully using @romandev 's fork in my own project now.

NickNaso · 2019-03-15T18:09:10Z

@KevinEady maybe you can start writing a draft to document this new api. What do you
think @romandev @mhdawson @gabrielschulhof ?

KevinEady · 2019-03-17T18:34:15Z

Hi @NickNaso , that sounds good. I have started on the docs, and I noticed this piece of code in the threadsafe_function.cc:

https://github.com/romandev/node-addon-api/blob/78b68bea4845413b9d06c584f8e21aa75e377058/test/threadsafe_function/threadsafe_function.cc#L39

This got me thinking about the context handling, and I noticed that we pass to napi_create_threadsafe_function a FinalizeData for the context. This means that the call_js callback will have this FinalizeData object passed as the context. Is this acceptable behavior?

KevinEady · 2019-03-18T12:47:11Z

This got me thinking about the context handling, and I noticed that we pass to napi_create_threadsafe_function a FinalizeData for the context. This means that the call_js callback will have this FinalizeData object passed as the context. Is this acceptable behavior?

I'm working on templating the ThreadSafeFunction class with the Context, and a no-Context tsfn will have nullptr_t as the Context type. With this, I can also implement the (currently unimplemented) GetContext(), as well as correctly passing the Context to CallJs callback.

I am also adding overloaded methods for ThreadSafeFunction<Context>::New() for the optional finalizer, data, and context.

If these API changes make sense, then that'll be what I'll document.

KevinEady · 2019-03-18T18:01:01Z

@romandev So I did the work on templating the Context, take a look here: https://github.com/romandev/node-addon-api/compare/threadfunction...KevinEady:threadfunction-context?expand=1

And it's used in the test class like https://github.com/KevinEady/node-addon-api/blob/8fc51e5cbeee30c6cdeac3fa0bc48c7cad9ef6d0/test/threadsafe_function/threadsafe_function.cc

What are your thoughts?

KevinEady · 2019-03-18T21:40:19Z

I'm currently working on the documentation https://github.com/KevinEady/node-addon-api/blob/threadfunction-doc/doc/threadsafe_function.md

DuBistKomisch · 2019-03-19T01:52:50Z

napi-inl.h

+                                                  size_t maxQueueSize,
+                                                  size_t initialThreadCount) {
+  return New(env, callback, Object(), resourceName, maxQueueSize,
+             initialThreadCount, nullptr);


nullptr used here (and elsewhere) is ambiguous so shows up as mismatched types ‘Context*’ and ‘std::nullptr_t’ when the intended overload is considered as a candidate

apparently need an explicitly overload with std::nullptr_t to resolve it, or I suppose cast it

DuBistKomisch · 2019-03-19T02:36:48Z

Also, the whole thing probably needs to be wrapped in a NAPI_VERSION check, explodes on 10.6 without declaring NAPI_EXPERIMENTAL.

mhdawson · 2019-03-19T17:36:25Z

@DuBistKomisch it probably needs a NAPI version 1.4 check instead as I believe the N-API methods it uses are non-experimental and that would check would exclude it from the earlier versions until we have the backports PRs landed.

mhdawson · 2019-03-19T17:36:50Z

Although maybe that is what you suggested :)

DuBistKomisch · 2019-03-19T23:36:47Z

yeah I'm no n-api expert but the node docs page doesn't seem to list a version under these thread safe function methods

mhdawson · 2019-03-20T21:59:10Z

hmm, we seem to be missing that info for those methods. I'll look at adding it as they should show as version 4.

KevinEady · 2019-03-25T10:10:54Z

@romandev will you be implementing a GetContext() function...?

napi-inl.h

Re-reviewed.

romandev · 2019-03-29T10:26:29Z

@KevinEady If you're already implementing GetContext(), I'll leave a FIXME comment and then you can land your patch in follow-up PR. If you'd like to include the GetContext() implementation in this patch, I'll continue the work. Basically, I wanted to implement GetContext without explicit template. (But I'm not sure if it is possible or not.)

gabrielschulhof · 2019-06-17T20:40:08Z

@romandev I think if we should throw when one attempts to assign to a non-empty ThreadSafeFunction.

romandev · 2019-06-18T19:49:44Z

@gabrielschulhof, In fact, I already did that here: https://github.com/nodejs/node-addon-api/pull/442/files#diff-f546e4cc35b454813e5264fe9c9e6fc8R3898

However, I think we still need a way to reset the ThreadSafeFunction to empty in ThreadSafeFinalizeCallback if the ThreadSafeFunction is aborted or released.

For example, in the current test, declare ThreadSafeFunction as a static global variable, and assign a new ThreadSafeFunction whenever StartThreadInternal is called. If it's non-empty at this time, it would throw an exception. However, if it's empty (by ThreadSafeFinalizeCallback), we can re-assign a new object again.

If there was no such logic, we would have to have a static ThreadSafeFunction array to create a new ThreadSafeFunction whenever StartThreadInternal is called. (because there would be no way to re-assign a new object) Or, we might need to make a wrapper object for ThreadSafeFunction on heap to maintain creating/destroying the object.

gabrielschulhof · 2019-06-19T17:06:38Z

@romandev a policy of "You cannot assign to a ThreadSafeFunction until the existing one has finished operating" makes sense to me.

Re: nodejs#442

mhdawson · 2019-06-28T13:59:56Z

test/threadsafe_function/threadsafe_function.cc

@@ -0,0 +1,185 @@
+#include <uv.h>


I think we want a big comment here that says this using uv is not guaranteed to be ABI stable. That if you want to use the stability guarantee from N-API you need to use other means to implement threads. This is the first example where we have used uv in the testing and we want it to be clear that it is not what we recommend and that we only do it because we know the test suite will be build/run with Node.js as opposed to being built into a binary that will run across different Node.js versions.

Hmmm ... we could use std::thread instead, right? I think all our supported compilers now have C++11, right? In the test in core I used libuv because, well, we maintain core ourselves, and because in core I seek to keep N-API tests written in C as much as possible. @romandev could you convert this to std::thread?

That would be better.

mhdawson

LGTM after the comment I suggested is added.

KevinEady · 2019-06-28T17:34:35Z

FWIW, I used `std::thread` in the example in the docs, so yeah makes sense to standardize.

…

On Fri, Jun 28, 2019, 7:27 PM Gabriel Schulhof ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/threadsafe_function/threadsafe_function.cc <#442 (comment)>: > @@ -0,0 +1,185 @@ +#include <uv.h> Hmmm ... we could use std::thread instead, right? I think all our supported compilers now have C++11, right? In the test in core I used libuv because, well, we maintain core ourselves, and because in core I seek to keep N-API tests written in C as much as possible. @romandev <https://github.com/romandev> could you convert this to std::thread? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#442?email_source=notifications&email_token=ACB4EIAFPSHSH63NBZLVXGTP4ZCWTA5CNFSM4GUNSRX2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5AIP6A#discussion_r298683140>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACB4EIGY5DYJY2NU5S4WHFTP4ZCWTANCNFSM4GUNSRXQ> .

gabrielschulhof · 2019-06-28T17:52:30Z

... and I'm actually thinking of using C11's thrd_create() in the core N-API test – wanna test though whether it's available on all our supported platforms.

gabrielschulhof · 2019-06-28T19:49:49Z

Well, it looks like #include <threads.h> doesn't work on all our platforms. Dunno if the STL is in a similar state. Maybe we should hold off on this for now and just bring in the comment from https://github.com/nodejs/node/blob/master/test/node-api/test_threadsafe_function/binding.c#L1-L5

gabrielschulhof · 2019-07-02T05:16:43Z

@addaleax:

[...] I think C++ std::thread is the easier way to go. That’s in C++11 and should be included everywhere.

@romandev can you please move the test over to std::thread?

romandev · 2019-07-02T08:22:02Z

@romandev can you please move the test over to std::thread?

@gabrielschulhof Done. Thanks.

gabrielschulhof · 2019-07-02T16:28:20Z

CI

Version	Job	Status
13	https://ci.nodejs.org/job/node-test-node-addon-api-new/139/	✓
12	https://ci.nodejs.org/job/node-test-node-addon-api-new/140/	✓
11	https://ci.nodejs.org/job/node-test-node-addon-api-new/141/	✗
10	https://ci.nodejs.org/job/node-test-node-addon-api-new/142/	✓
9	https://ci.nodejs.org/job/node-test-node-addon-api-new/143/	✗
8	https://ci.nodejs.org/job/node-test-node-addon-api-new/144/	✗

This PR is implementing ThreadSafeFunction class wraps napi_threadsafe_function features. FYI, the test files that included in this PR have come from Node.js repo[1]. They've been rewritten based on C++ and node-addon-api. Fixes nodejs#312. [1] https://github.com/nodejs/node/tree/master/test/node-api/test_threadsafe_function

romandev · 2019-07-03T05:58:46Z

FYI, I got rid of std::make_unique from latest patch because the make_unique() is C++ 14 feature..

gabrielschulhof · 2019-07-09T22:03:04Z

New CI:

Version	Job	Status
13	https://ci.nodejs.org/job/node-test-node-addon-api-new/194/	✓
12	https://ci.nodejs.org/job/node-test-node-addon-api-new/195/	✓
10	https://ci.nodejs.org/job/node-test-node-addon-api-new/197/	✓
8	https://ci.nodejs.org/job/node-test-node-addon-api-new/199/	✓

*edit: dropped non-LTS.

KevinEady · 2019-07-10T15:51:36Z

Hi @gabrielschulhof ,

Looks like the node-9 tests are failing because the napi tsfn functions don't exist. Were tsfns backported to node 9? Don't see them listed on the n-api docs.

This PR is implementing ThreadSafeFunction class wraps napi_threadsafe_function features. FYI, the test files that included in this PR have come from Node.js repo[1]. They've been rewritten based on C++ and node-addon-api. [1] https://github.com/nodejs/node/tree/e800f9d/test/node-api/test_threadsafe_function PR-URL: #442 Fixes: #312 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>

gabrielschulhof · 2019-07-10T17:35:13Z

Landed in 0a90df2. Thanks @romandev!

This PR is implementing ThreadSafeFunction class wraps napi_threadsafe_function features. FYI, the test files that included in this PR have come from Node.js repo[1]. They've been rewritten based on C++ and node-addon-api. [1] https://github.com/nodejs/node/tree/e800f9d/test/node-api/test_threadsafe_function PR-URL: nodejs/node-addon-api#442 Fixes: nodejs/node-addon-api#312 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>

gabrielschulhof previously requested changes Feb 15, 2019

View reviewed changes

napi.h Outdated Show resolved Hide resolved

test/threadsafe_function/threadsafe_function.cc Outdated Show resolved Hide resolved

test/threadsafe_function/threadsafe_function.cc Outdated Show resolved Hide resolved

test/threadsafe_function/threadsafe_function.js Show resolved Hide resolved

legendecas reviewed Feb 15, 2019

View reviewed changes

test/threadsafe_function/threadsafe_function.cc Outdated Show resolved Hide resolved

romandev force-pushed the threadfunction branch from 3d02ab3 to cf15f53 Compare March 6, 2019 09:26

romandev changed the title ~~[WIP] Implement ThreadSafeFunction class~~ Implement ThreadSafeFunction class Mar 6, 2019

romandev force-pushed the threadfunction branch 2 times, most recently from a78aff8 to 78b68be Compare March 6, 2019 15:50

KevinEady reviewed Mar 11, 2019

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

mhdawson mentioned this pull request Mar 11, 2019

Time for a new release? #454

Closed

4 tasks

romandev force-pushed the threadfunction branch from 78b68be to f04ba10 Compare March 18, 2019 16:56

DuBistKomisch reviewed Mar 19, 2019

View reviewed changes

romandev force-pushed the threadfunction branch from f04ba10 to 5fc684d Compare March 22, 2019 07:24

NickNaso mentioned this pull request Mar 22, 2019

async thread that emittes data #462

Closed

gabrielschulhof reviewed Mar 25, 2019

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

napi-inl.h Outdated Show resolved Hide resolved

napi-inl.h Outdated Show resolved Hide resolved

napi-inl.h Outdated Show resolved Hide resolved

gabrielschulhof approved these changes Jun 19, 2019

View reviewed changes

mhdawson mentioned this pull request Jun 24, 2019

Passing char pointer #501

Closed

KevinEady added a commit to KevinEady/node-addon-api that referenced this pull request Jun 26, 2019

doc: document ThreadSafeFunction

186ebf2

Re: nodejs#442

mhdawson reviewed Jun 28, 2019

View reviewed changes

mhdawson approved these changes Jun 28, 2019

View reviewed changes

gabrielschulhof mentioned this pull request Jul 2, 2019

test: remove thread-safe function uv thread usage nodejs/node#28472

Closed

3 tasks

romandev force-pushed the threadfunction branch from 86223cf to 19c115d Compare July 2, 2019 08:16

gabrielschulhof approved these changes Jul 2, 2019

View reviewed changes

romandev force-pushed the threadfunction branch from 19c115d to d534bdf Compare July 3, 2019 05:56

gabrielschulhof mentioned this pull request Jul 10, 2019

How do I save a callback for later with node-addon-api? #432

Closed

gabrielschulhof closed this Jul 10, 2019

Implement ThreadSafeFunction class #442

Implement ThreadSafeFunction class #442

Conversation

romandev commented Feb 5, 2019 • edited Loading

mhdawson commented Feb 6, 2019

gabrielschulhof left a comment

Choose a reason for hiding this comment

romandev commented Mar 6, 2019

KevinEady left a comment

Choose a reason for hiding this comment

mhdawson commented Mar 12, 2019

KevinEady commented Mar 15, 2019

NickNaso commented Mar 15, 2019

KevinEady commented Mar 17, 2019

KevinEady commented Mar 18, 2019

KevinEady commented Mar 18, 2019

KevinEady commented Mar 18, 2019

DuBistKomisch Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

DuBistKomisch commented Mar 19, 2019

mhdawson commented Mar 19, 2019

mhdawson commented Mar 19, 2019

DuBistKomisch commented Mar 19, 2019

mhdawson commented Mar 20, 2019

KevinEady commented Mar 25, 2019

romandev commented Mar 29, 2019

gabrielschulhof commented Jun 17, 2019

romandev commented Jun 18, 2019

gabrielschulhof commented Jun 19, 2019

mhdawson Jun 28, 2019

Choose a reason for hiding this comment

gabrielschulhof Jun 28, 2019

Choose a reason for hiding this comment

mhdawson Jun 28, 2019

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

KevinEady commented Jun 28, 2019 via email

gabrielschulhof commented Jun 28, 2019

gabrielschulhof commented Jun 28, 2019

gabrielschulhof commented Jul 2, 2019

romandev commented Jul 2, 2019

gabrielschulhof commented Jul 2, 2019 • edited Loading

romandev commented Jul 3, 2019

gabrielschulhof commented Jul 9, 2019 • edited Loading

KevinEady commented Jul 10, 2019

gabrielschulhof commented Jul 10, 2019

romandev commented Feb 5, 2019 •

edited

Loading

DuBistKomisch Mar 19, 2019 •

edited

Loading

gabrielschulhof commented Jul 2, 2019 •

edited

Loading

gabrielschulhof commented Jul 9, 2019 •

edited

Loading