AsyncProgressQueueWorker::Signal() does not trigger the TSFN

[KevinEady]

As reported in #1086 (comment)

[legendecas]

Linking to #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:

node-addon-api/napi-inl.h

Lines 6010 to 6013 in e439222

	template<class T>
	inline void AsyncProgressQueueWorker<T>::ExecutionProgress::Signal() const {
	_worker->Signal();
	}

node-addon-api/napi-inl.h

Lines 5999 to 6002 in e439222

	template<class T>
	inline void AsyncProgressQueueWorker<T>::Signal() const {
	this->NonBlockingCall(nullptr);
	}

node-addon-api/napi-inl.h

Lines 5736 to 5740 in e439222

	template <typename DataType>
	inline napi_status AsyncProgressWorkerBase<DataType>::NonBlockingCall(DataType* data) {
	auto tsd = new AsyncProgressWorkerBase::ThreadSafeData(this, data);
	return _tsfn.NonBlockingCall(tsd, OnAsyncWorkProgress);
	}

node-addon-api/napi-inl.h

Lines 5727 to 5734 in e439222

	template <typename DataType>
	inline void AsyncProgressWorkerBase<DataType>::OnAsyncWorkProgress(Napi::Env /* env */,
	Napi::Function /* jsCallback */,
	void* data) {
	ThreadSafeData* tsd = static_cast<ThreadSafeData*>(data);
	tsd->asyncprogressworker()->OnWorkProgress(tsd->data());
	delete tsd;
	}

node-addon-api/napi-inl.h

Lines 5976 to 5981 in e439222

	template<class T>
	inline void AsyncProgressQueueWorker<T>::OnWorkProgress(std::pair<T*, size_t>* datapair) {
	if (datapair == nullptr) {
	return;
	}

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 #821 (comment). 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]

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]

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]

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]

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