AsyncProgressQueueWorker::Signal() does not trigger the TSFN

As reported in #1086 (comment)

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.

@legendecas

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.

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

@KevinEady

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

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.

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.

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.

Fixed by #1216

Navigation Menu

Repository navigation

AsyncProgressQueueWorker::Signal() does not trigger the TSFN #1095

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

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

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

	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;
	}

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

AsyncProgressQueueWorker::Signal() does not trigger the TSFN #1095

Description

Activity

legendecas commented on Oct 26, 2021

KevinEady commented on Nov 5, 2021

KevinEady commented on Nov 5, 2021

legendecas commented on Nov 8, 2021

github-actions commented on Feb 7, 2022

KevinEady commented on Feb 7, 2022

github-actions commented on May 9, 2022

github-actions commented on Aug 29, 2022

github-actions commented on Dec 28, 2022

KevinEady commented on Dec 29, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions