Repository navigation
Issues with the new ThreadSafeFunction api #524
Description
Activity
@extmchristensen, @gabrielschulhof who it probably the best person to comment is away for another week so it might be after that when we engage in a discussion on this.
Any news on when the copy constructor will be implemented?
Having the same issue as 1), I tried to convert the lambda callback to a functor that was going to hold the ThreadSafeFunction, but for some reason I didn't manage to get it to compile.
I also posted another question on why the callback Function is a required parameter on any ThreadSafeFunction::New(), I use Promises and having to pass a Function is redundant to me. It could be optional or I could be able to choose between Function and Promise, no?- Most importantly, the ThreadSafeCallback class is lacking a copy-constructor so I can not safely and easily capture it into c++ threads, lambdas and AsyncWorker's with pass by value. The provided example at the bottom of "https://github.com/nodejs/node-addon-api/blob/master/doc/threadsafe_function.md" solves this by having ThreadSafeCallback as a global mutable variable but this is dangerous (might break with repeated calls to the native function quickly after each other) and I also personally think global mutable variables is a bad thing. Instead the native method should initialize the ThreadSafeCallback object which is than copied into the thread/lambda/worker by value (before original object is destroyed as it goes out of scope when function returns etc).
- std::unique_ptr<napi_threadsafe_function, Deleter> _tsfn; means we cannot just add a copy constructor.
- Seems like we can just make it a regular pointer.
- We should add acquire on all creates, and release on all releases (means acquire/release won't necessarily need to be called at all)
- Creating a copy should increase tfsn ref count, requiring release for each copy.
- Assignment should do release.
@mhdawson / @gabrielschulhof ,
I was thinking about what we said regarding these points:
We should add acquire on all creates, and release on all releases (means acquire/release won't necessarily need to be called at all)
Creating a copy should increase tfsn ref count, requiring release for each copy.
Assignment should do release.I don't feel like it is necessary to tie acquire & release to copy + destruction... What was the reasoning behind this? I think it had something to do with race conditions? Would these race conditions still exist if we were using a normal napi
napi_threadsafe_functionand not the wrapper? If so, I feel like it's something we do not need to specifically worry about in node-addon-api.Likewise, with the concern of overwriting a
ThreadSafeFunctionwith an existing one, a la:ThreadSafeFunction tsfn; tsfn = ThreadSafeFunction::New(...); tsfn = ThreadSafeFunction::New(...);
... would end up leaking the first TSFN... I feel like we should not need to specifically handle this case... eg, what would happen in the regular napi case?
napi_threadsafe_function tsfn; napi_create_threadsafe_function(..., &tsfn); napi_create_threadsafe_function(..., &tsfn);
.. the first TSFN reference is lost, no?
Going back to @gabrielschulhof 's overall mantra of "the TSFN wrapper shouldn't have too much logic" ... using the ThreadSafeFunction correctly (eg don't try to copy it into another thread when the tsfn is no longer available) would be up to the developer, which in a lot of instances it already is anyway (eg. if you get a
napi_closingfrom aBlockingCall, don't make any more calls)One thing I see (which I think we brought up) is that the copied
ThreadSafeFunctions will point to an invalid_tsfnafter theFinalizeis called ... but i'm not 100% sure this is actually a problem (going back to "using the ThreadSafeFunction correctly would be up to the developer")?@KevinEady I understand your point, but @extmchristensen 1. point is real, since I am using ThreadSafeFunction together with a Promise, if I chain two calls in the JS that will use the same
tsfnlike in your example here:
https://github.com/nodejs/node-addon-api/blob/master/doc/threadsafe_function.md#exampleThe main problem is my code calls Promise.Resolve() before
tsfn.Release();and in the JavaScript resolution function I call another add-on API which reassign tfsn and produces the following crash:
FATAL ERROR: ThreadSafeFunction::operator = You cannot assign a new TFSN because existing one is still alive.So the example doesn't really work in real life code, unless I am missing something.
@marco-ms Sorry, I'm just trying to understand your use-case... Why are you re-assigning tsfn?
I have been following the example where a global tfsn is created and reused. However your example is pretty basic and just throws result every second for N times (N defined by JavaScript caller).What I have been doing is exposing two functions:
aandb.
Bothaandbreturns a promise with a result, they don't use the callback in the ThreadSafeFunction.
Bothaandbuses globaltsfnand redefine it, but if I chainaandblike this:b().then(() => { // tsfn has called our callback which has resolved the promise a().then(() => { // it'll crash just calling a() since it'll reassign tsfn before b is done with it and call Release() }); )};It'll crash since b calls blocking until JavaScript code is done, a's kicks in and will redefine tfsn and will crash with the error mentioned above.
So I tried to wrap ThreadSafeFunction like suggested by @extmchristensen here is the code:#include "MyThreadSafeFunction.h" MyThreadSafeFunction::MyThreadSafeFunction(Napi::Env env) { // https://github.com/nodejs/node-addon-api/blob/master/doc/threadsafe_function.md // This API for thread safety requires N-API v4 which is supported only by the following Node: // v8.16.0, v11.8.0, v12.0.0 (or greater) // https://nodejs.org/api/n-api.html#n_api_n_api_version_matrix Napi::Function bogusCallback = Napi::Function::New(env, [](const Napi::CallbackInfo& cbinfo) { return cbinfo.Env().Undefined(); } ); _tsfn = Napi::ThreadSafeFunction::New( env, bogusCallback, // We use Promises, not Callbacks "Resource Name", // Name 0, // Unlimited queue 2 // Does it matter? ); } MyThreadSafeFunction::~MyThreadSafeFunction() { _tsfn.Release(); } napi_status MyThreadSafeFunction::BlockingCall(function<void(Napi::Env env, Napi::Function jsCallback)> callback) { return _tsfn.BlockingCall(callback); } napi_status MyThreadSafeFunction::NonBlockingCall(function<void(Napi::Env env, Napi::Function jsCallback)> callback) { return _tsfn.NonBlockingCall(callback); }This way each time I call a method it'll have its own tsfn instance and will be released during dtor, however for some reason Electron is now crashing randomly with:
Electron(21589,0x1198705c0) malloc: Incorrect checksum for freed object 0x7fd68cb12460: probably modified after being freed. Corrupt value: 0x0 Electron(21589,0x1198705c0) malloc: *** set a breakpoint in malloc_error_break to debugAnd I haven't figured out why.
Hi @gabrielschulhof ,
I'm working on the copy constructor, and I'm thinking back to something you said regarding the underlying
napi_threadsafe_function, is that it itself acts as a "pointer" . If that is the case, could theThreadSafeFunctionclass just have the concretenapi_threadsafe_function _tsfninstead of a pointer to one? Thereby, copying aThreadSafeFunctionwould copy whole napi_threadsafe_function structure in the new object. Is this okay? How would it behave in the C world if we just copiednapi_threadsafe_functions?I'm concerned that if we're using pointers, we'll need to take special care of the memory, as
::Newneeds to create a new object (which eventually needs to be deleted), whereas theThreadSafeFunctionalso has a constructor taking an existingnapi_threadsafe_function, which shouldn't be deleted.- added a commit that references this issue
on Sep 18, 2019 - added a commit that references this issue
on Nov 1, 2019 - It is not clear what initialThreadCount should be set to
Take a look at this post. In all actuality, the thread count is just that: a counter. Even if you set it to
1, you could theoretically have 10 threads using it (but then you'd need to implement some lifecycle management yourself). As long as no call toAcquire()is made, this tsfn would destroy as soon asRelease()(orAbort()) is called.In the specific example, it is set to
1because only one thread does use it: the thread started viastd::thread. That thread is also the only one that callsRelease(), which then decreases the count to0, causing the finalization to begin.- There is no way for the C++ code to wait for the queue to be empty
I feel like this can be solved programmatically by the developer...? If the last item you add to the queue performs some callback, you can perform some logic there. The underlying N-API does not have any functionality for this, so I don't think node-addon-api can provide this.
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
I'm going to close this since its been a long time since the last response. Please let us know if you think that was not the right thing to do.
- added a commit that references this issue
on Aug 24, 2022 - added a commit that references this issue
on Aug 26, 2022 - added a commit that references this issue
on Sep 19, 2022 - added a commit that references this issue
on Aug 11, 2023
@romandev I am enthusiastically trying to use the new ThreadSafeFunction Api as a replacement for the non-standard napi-thread-safe-callback which I was using previously. I have a few problems though in my (typical?) use case where I have async native C++ functions that return quickly to JS but continue to do the actual work in an async C++ worker or thread, calling back into JS with progress information and/or result(s) using ThreadSafeFunction.
NB: Updated to show that issue 1 & 2 is now solved
1) SOLVED: Most importantly, the ThreadSafeCallback class is lacking a copy-constructor so I can not safely and easily capture it into c++ threads, lambdas and AsyncWorker's with pass by value. The provided example at the bottom of "https://github.com/nodejs/node-addon-api/blob/master/doc/threadsafe_function.md" solves this by having ThreadSafeCallback as a global mutable variable but this is dangerous (might break with repeated calls to the native function quickly after each other) and I also personally think global mutable variables is a bad thing. Instead the native method should initialize the ThreadSafeCallback object which is than copied into the thread/lambda/worker by value (before original object is destroyed as it goes out of scope when function returns etc).
2) FIXED BY ABOVE: In the absence of a copy-constructor, it is a problem that the ThreadSafeCallback class has only factory methods (for parameters) and lacks a constructor with the various parameters so new/delete could be used to control the life-cycle and hereby enable capturing of a pointer to ThreadSafeFunction in c++ threads, workers and lambdas (As a temporary fix, I have created by own simple wrapper class that holds a ThreadSafeFunction inside and has a constructor with all the arguments thus enabling the wrapper with the ThreadSafeFunction to be created on the heap).
3) It is not clear what initialThreadCount should be set to and how and when Acquire and Release should be called. The example sets initialThreadCount to 1 though used by two threads and does not call Acquire ? From my own experiments, it seems important to call Release exactly once to make sure the node process terminates - it does not seem to matter what Thread I call Release from in my example as long as I don't call it twice (in which case an Assert error with a nice stack trace will be shown).
4 There is no way for the C++ code to wait for the queue to be empty (all calls in queue to be processed). This would be useful to delay cleanup until a NonBlockingCall has been fully executed.