AsyncProgressQueueWorker::Signal() does not trigger the TSFN

[KevinEady]

As reported in https://github.com/nodejs/node-addon-api/pull/1086#issue-1017797535

[legendecas]

Linking to https://github.com/nodejs/node-addon-api/pull/853 as it introduced the problem of discarding nullptr data while process progress, whilst Signal sends a nullptr data to signal the worker.

[KevinEady]

Hi @legendecas , can you elaborate some more on the problem introduced in 853? Maybe we can discuss in today's call.

EDIT: Ah, I see it now:

https://github.com/nodejs/node-addon-api/blob/e439222fe64e941dfeeb9d865d05214ab8770a0c/napi-inl.h#L6010-L6013

https://github.com/nodejs/node-addon-api/blob/e439222fe64e941dfeeb9d865d05214ab8770a0c/napi-inl.h#L5999-L6002

https://github.com/nodejs/node-addon-api/blob/e439222fe64e941dfeeb9d865d05214ab8770a0c/napi-inl.h#L5736-L5740

https://github.com/nodejs/node-addon-api/blob/e439222fe64e941dfeeb9d865d05214ab8770a0c/napi-inl.h#L5727-L5734

https://github.com/nodejs/node-addon-api/blob/e439222fe64e941dfeeb9d865d05214ab8770a0c/napi-inl.h#L5976-L5981

I have been able to resolve via:

@@ -5986,7 +5990,8 @@ inline void AsyncProgressQueueWorker<T>::SendProgress_(const T* data, size_t cou
 
 template<class T>
 inline void AsyncProgressQueueWorker<T>::Signal() {
-  this->NonBlockingCall(nullptr);
+  auto pair = new std::pair<T*, size_t>(nullptr, 0);
+  this->NonBlockingCall(pair);
 }

If this is the correct implementation, I can create a PR. Let's discuss in today's call.

[KevinEady]

😅 The above fix might be a bit premature. Let's discuss in the call.

[legendecas]

@KevinEady the problem introduced in #853 is that it unconditionally suppressed the calls when the data is nullptr, for timing details you can checkout the graph here https://github.com/nodejs/node-addon-api/issues/821#issuecomment-723626374. So I believe we should try to find a way to distinguish Signals (a user-initiated action) from duplicated activation from second uv idle callback (not a user-initiated action).

Signal is a user-initiated action that will trigger the worker to call the progress callback even if there is no progress data available. Yet, if there is any progress data available, Signal just do nothing. For reference, AsyncProgressWorker and AsyncProgressQueueWorker is designed to be compatible (in our best effort) with nan's AsyncProgressWorker.

[github-actions[bot]]

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.

[KevinEady]

Current state: I have an implementation that solves this issue, but it suffers from the "not all of the async Signal() calls (which calls the TSFN NonBlockingCall()) get executed. We discussed in 28 Jan meeting to port the implementation over to Node-API directly (without using the node-addon-api wrapper) to see if it's an issue with the underlying thread-safe functions APIs or with the wrapper. This is currently ongoing.

[github-actions[bot]]

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]]

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]]

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.

[KevinEady]

Fixed by #1216