Remove const from AsyncProgressWorker::Signal and AsyncProgressQueueWorker::Signal

[JosephusPaye]

This PR fixes#1081 by removing const from the AsyncProgressWorker::Signal and AsyncProgressQueueWorker::Signal methods, and also from the ExecutionProgress parameter of the Execute method (this one might be a breaking change).

Also added a test for the AsyncProgressWorker::Signal. AsyncProgressQueueWorker::Signal needs to be tested as well. EDIT: Test added, but AsyncProgressQueueWorker::Signal()doesn't work, needs fixing.

[KevinEady]

Hi @JosephusPaye ,

Thanks for your PR! We discussed this inside today's Node.js API meeting. Looking back at your original issue, you say the const issue comes from crossing the boundary between points 2 and 3:

// 2. https://github.com/nodejs/node-addon-api/blob/4351bffd537eab927226bf6f9b66cd385a049a43/napi-inl.h#L5876template<classT>
inlinevoid AsyncProgressWorker<T>::Signal() const {
this->NonBlockingCall(static_cast<T*>(nullptr));
}
// 3. https://github.com/nodejs/node-addon-api/blob/4351bffd537eab927226bf6f9b66cd385a049a43/napi-inl.h#L5725template <typename DataType>
inline napi_status AsyncProgressWorkerBase<DataType>::NonBlockingCall(DataType* data) {
auto tsd = newAsyncProgressWorkerBase::ThreadSafeData(this, data);
return _tsfn.NonBlockingCall(tsd, OnAsyncWorkProgress);
}

Have you thought about making the method in point three const instead? Does this fix the issue as well?

Thanks, Kevin

[JosephusPaye]

Hi Kevin,

I went ahead and tried adding const to NonBlockingCall() but that raises further errors that require adding const in other places:

• AsyncProgressWorkerBase::ThreadSafeData needs const on the constructor param that takes the AsyncProgressWorkerBase instance, the member variable that stores it, and the method that returns it
• As a result OnWorkProgress() needs const on AsyncProgressWorkerBase, AsyncProgressWorker and AsyncProgressQueueWorker
• As a result OnProgress() needs const (this one is a user-visible change, potentially breaking)
• This breaks the AsyncProgressQueueWorker test, where the OnProgress() method calls the Receiver() method to run a JS callback, and also calls notify_one() on the std::condition_variable member variable, where notify_one() is non-const.

[JosephusPaye]

It also appears AsyncProgressQueueWorker::Signal() is not working as expected, as this test shows.

[KevinEady]

Hi @JosephusPaye ,

We reviewed both of your issues here (the const and the non-call of Signal()). We would like to separate them into two different issues, so I created the non-calling of Signal() here #1095

@legendecas will take a further look into the implementation of why const is failing here, because our implementation is based off of Nan's which is const.

[JosephusPaye]

Sounds good!