node-addon-api icon indicating copy to clipboard operation
node-addon-api copied to clipboard

Handling event through JavaScript function when it is emitted from C++

Open GaurangTandon opened this issue 3 years ago • 10 comments

I have attached an MVCE here: napi-mvce.zip To run, yarn && yarn run build:debug && node ./index.js.

The relevant code snippet is:

function processEvent(data) {
    console.log('Process event data', data);
}

project.startListeners(function eventHandler(data) {
    setTimeout(processEvent, 100, data); // process in separate "thread"
    // processEvent(data); // process in same "thread"
    return 0;
});

On the C++ side:

volatile long long int i = 0;

Napi::Number _startListeners(const Napi::CallbackInfo &info) {
    Napi::Env env = info.Env();

    auto callback = info[0].As<Napi::Function>();

    while (true) {
        if (i == 1e6) {
            Napi::Number num = Napi::Number::New(env, 10);
            callback.Call({num});
            i = 0;
        }
        i++;
    }

    return Napi::Number::New(env, 0);
}

As you may guess, I'm trying to build an event handler, where the event is emitted from the C++ layer using a for loop based polling strategy, and then the event is handled by a function in JavaScript. The event handler should be in a separate "thread" in JavaScript so as to not block the event emitter loop.

This desired functionality is not being achieved. When I use setTimeout, it appears that processEvent is not being called. Without setTimeout, processEvent does get called as I see the output on the terminal.

Even though I have written this code, I have doubts on its correctness, because it may result in events being handled out of order (as setTImeout does not give an exact timing guarantee)

This question might stem from an improper understanding of NAPI, so I may be grateful if instead of suggesting a fix to the above architecture you may suggest a different architecture altogether for handling such an event setup.

The ideal setup would be a producer/consumer queue architecture where consumer and producer are in different threads each, but I am not sure to perform that.

GaurangTandon avatar Jan 21 '22 07:01 GaurangTandon

You should look at

https://github.com/nodejs/node-addon-api/blob/main/doc/threadsafe.md

and

https://github.com/nodejs/node-addon-api/blob/main/doc/async_worker.md

as options.

mhdawson avatar Jan 24 '22 23:01 mhdawson

Hi @GaurangTandon ,

Does Michael's suggestion above provide you with any information? Do you need any further assistance?

KevinEady avatar Feb 11 '22 16:02 KevinEady

Hi, I tried once and was not able to get it working. Although I was not able to go through it in detail yet. I hope to do that soon. Thanks for following up!

GaurangTandon avatar Feb 11 '22 16:02 GaurangTandon

Hi @KevinEady , apologies for the late followup. I tried Michael's suggestion, and I got an output that I am unable to explain.

tl;dr: The TSFN [Non]BlockingCall() call is waiting for the main native addon thread to exit before calling the JS function. This is unexpected because I made the method call is in a separate thread (which is not the main thread). I had wanted the JS function to be executed in parallel with the main thread, but it is not happening.

Relevant native addon code: (link to updated mvce.zip)

    bool first_thread = true;
    // generate four events, one per second
    for (int i = 0; i < 4; i++) {
        nativeThreads.push_back(std::thread([i, first_thread]() {
            if (!first_thread)
                tsfn.Acquire();

            std::cout << "Started thread " << i << std::endl;
            
            tsfn.NonBlockingCall(new int(clock()));
            tsfn.Release();
        }));

        first_thread = false;

        std::this_thread::sleep_for(std::chrono::milliseconds(1000));
    }

Corresponding JavaScript code (processEvent is the callback to TSFN)

let index = 1;
project.startListeners(function processEvent(data) {
    const receiveTime = Date.now();
    // simulate 500ms CPU intensive task
    while (Date.now() - receiveTime <= 500)
        continue;
    console.log('-- JS: index', index++, '; data', data);
});

I have initialized a very large queue for the TSFN (20 items). I tried with both BlockingCall and NonBlocking call, and get same result.

Expected output:

Started thread 0
-- JS: index 1 ; data 0
Started thread 1
-- JS: index 2 ; data 1005
Started thread 2
-- JS: index 3 ; data 2006
Started thread 3
EXITED main function
-- JS: index 4 ; data 3006
Exited thread 1
Exited thread 2
Exited thread 3
Exited thread 4

Actual output

Started thread 0
Started thread 1
Started thread 2
Started thread 3
EXITED main function
-- JS: index 1 ; data 0
-- JS: index 2 ; data 1005
-- JS: index 3 ; data 2006
-- JS: index 4 ; data 3006
Exited thread 1
Exited thread 2
Exited thread 3
Exited thread 4

Notice how all the JS method calls execute after the main function has exited. Could you please let me know if this is intended?

For now, as a workaround, I am thinking of creating multiple events inside a single long-lived thread, rather than creating one short-lived thread per event.

GaurangTandon avatar Feb 15 '22 05:02 GaurangTandon

This is expected behavior. Your _startListeners function will create the threads and add a new "request to execute code" via the NonBlockingCall at each thread run, but the actual execution of the code will not happen until a subsequent tick of the event loop. Since your _startListeners is still actively running when the NonBlockingCall is executed, the request goes into the queue and waits for the next tick, which occurs after _startListeners finishes (ie. the "EXITED main function") string. The next tick occurs, so now your TSFN's queue can be worked on. Your "Exited thread <n>" lines come all at once due to the cout being in the finalizer of the TSFN -- it's a little misleading because the thread is actually exited prior (after line 47, tsfn.Release();)

KevinEady avatar Feb 15 '22 09:02 KevinEady

Thanks Kevin for your detailed reply! I really appreciate it, and the event loop example makes sense to me. I have two follow up requests with regards to the documentation of this feature:

  1. May we add this unexpected scenario to the addon examples, perhaps to https://github.com/nodejs/node-addon-examples/tree/main/typed_threadsafe_function/node-addon-api or some other more suitable location as suggested by you. I think there would be other folks like me who would be confused at this behavior. I can help create a PR for the same if you wish.
  2. What is the MakeCallback method on Napi::Function? I could not find any docs on its example use case. The corresponding example file does not cover it. Could you describe its use case?

Thank you for your help!

GaurangTandon avatar Feb 16 '22 06:02 GaurangTandon

Hi @GaurangTandon ,

We discussed in the 18 March Node API meeting:

  1. It would be beneficial to update the TSFN documentation to explain the above behavior, in that the current execution cycle where the TSFN gets created must exit before the TSFN's queue can be processed.
  2. The Function documentation outlines that MakeCallback should be used when calling JS from an asynchronous operation, whereas:

The Call method is used when there is already a JavaScript function on the stack (for example when running a native method called from JavaScript).

KevinEady avatar Mar 18 '22 15:03 KevinEady

Thank you, so should I keep this issue open while item number 1 remains unresolved?

GaurangTandon avatar Mar 19 '22 22:03 GaurangTandon

@KevinEady is adding the doc suggested something you might do in the near future?

mhdawson avatar May 05 '22 19:05 mhdawson

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.

github-actions[bot] avatar Aug 04 '22 00:08 github-actions[bot]