const error with AsyncProgressWorker<T>::ExecutionProgress::Signal()

[JosephusPaye]

Hi 👋

I'm getting the following error trying to call .Signal() on AsyncProgressWorker::ExecutionProgress:

View error

In file included from /home/jpaye/nuclearnet.js/node_modules/node-addon-api/napi.h:2905,
                 from ../src/NetworkBinding.hpp:22,
                 from ../src/NetworkListener.hpp:21,
                 from ../src/NetworkListener.cpp:18:
/home/jpaye/nuclearnet.js/node_modules/node-addon-api/napi-inl.h: In instantiation of ‘void Napi::AsyncProgressWorker<T>::Signal() const [with T = char]’:
/home/jpaye/nuclearnet.js/node_modules/node-addon-api/napi-inl.h:5797:12:   required from ‘void Napi::AsyncProgressWorker<T>::ExecutionProgress::Signal() const [with T = char]’
../src/NetworkListener.cpp:83:22:   required from here
/home/jpaye/nuclearnet.js/node_modules/node-addon-api/napi-inl.h:5792:3: error: passing ‘const Napi::AsyncProgressWorker<char>’ as ‘this’ argument discards qualifiers [-fpermissive]
 5792 |   this->NonBlockingCall(static_cast<T*>(nullptr));
      |   ^~~~
In file included from /home/jpaye/nuclearnet.js/node_modules/node-addon-api/napi.h:2905,
                 from ../src/NetworkBinding.hpp:22,
                 from ../src/NetworkListener.hpp:21,
                 from ../src/NetworkListener.cpp:18:
/home/jpaye/nuclearnet.js/node_modules/node-addon-api/napi-inl.h:5640:20: note:   in call to ‘napi_status Napi::AsyncProgressWorkerBase<DataType>::NonBlockingCall(DataType*) [with DataType = void]’
 5640 | inline napi_status AsyncProgressWorkerBase<DataType>::NonBlockingCall(DataType* data) {
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make: *** [nuclearnet.target.mk:119: Release/obj.target/nuclearnet/src/NetworkListener.o] Error 1
make: Leaving directory '/home/jpaye/nuclearnet.js/build'

From reading the code, this is what I've been able to piece together this sequence of calls:

// 0. My code, p is a const Napi::AsyncProgressWorker<T>::ExecutionProgress&
// https://github.com/Fastcode/NUClearNet.js/blob/ecf4d9005feaad38bc139924d28de6a67b88e999/src/NetworkListener.cpp#L83
p.Signal();

// 1. https://github.com/nodejs/node-addon-api/blob/4351bffd537eab927226bf6f9b66cd385a049a43/napi-inl.h#L5881
template<class T>
inline void AsyncProgressWorker<T>::ExecutionProgress::Signal() const {
  _worker->Signal();
}

// 2. https://github.com/nodejs/node-addon-api/blob/4351bffd537eab927226bf6f9b66cd385a049a43/napi-inl.h#L5876
template<class T>
inline void AsyncProgressWorker<T>::Signal() const {
  this->NonBlockingCall(static_cast<T*>(nullptr));
}

// 3. https://github.com/nodejs/node-addon-api/blob/4351bffd537eab927226bf6f9b66cd385a049a43/napi-inl.h#L5725
template <typename DataType>
inline napi_status AsyncProgressWorkerBase<DataType>::NonBlockingCall(DataType* data) {
  auto tsd = new AsyncProgressWorkerBase::ThreadSafeData(this, data);
  return _tsfn.NonBlockingCall(tsd, OnAsyncWorkProgress);
}

It appears the jump from 2 to 3 discards const on this, causing the error.

The specific line where I call .Signal() in my code is available here along with the rest of the code for context.

[mhdawson]

@JosephusPaye thanks for reporting. I looked at our test suite as I wondered why we would not have caught it earlier. It seems we don't cover this case. Any chance you would be interested in submitting a PR with a fix and a test case?

@gabrielschulhof my first thought is we should remove the const from Signal. That might be considered a breaking change but if code calling Signal would not have compiled before due to the call to the non-const NonBlockingCall then I don't think we need to treat it like one.

[JosephusPaye]

@JosephusPaye thanks for reporting. I looked at our test suite as I wondered why we would not have caught it earlier. It seems we don't cover this case. Any chance you would be interested in submitting a PR with a fix and a test case?

Yes, I can do so. I'll have a look soon.

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

[JosephusPaye]

There's a pending PR to fix this issue: #1086

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

[JosephusPaye]

Not resolved yet.

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

[JosephusPaye]

Not resolved yet (as far as I can tell).

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

[JosephusPaye]

Was this confirmed as fixed as of #1216?

[mhdawson]

I think so but @KevinEady can you confirm?

[KevinEady]

Yep. Closing issue.

[JosephusPaye]

Thank you both!