Skip to content

Issues with the new ThreadSafeFunction api #524

Closed
@ghost

Description

@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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions