Repository navigation
const error with AsyncProgressWorker<T>::ExecutionProgress::Signal() #1081
Description
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.
Activity
mhdawson commentedon Oct 1, 2021
@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 commentedon Oct 2, 2021
Yes, I can do so. I'll have a look soon.
github-actions commentedon Jan 1, 2022
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 commentedon Jan 3, 2022
There's a pending PR to fix this issue: #1086
github-actions commentedon Apr 4, 2022
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 commentedon Jul 4, 2022
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 commentedon Jul 6, 2022
Not resolved yet.
github-actions commentedon Oct 5, 2022
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 commentedon Oct 5, 2022
Not resolved yet (as far as I can tell).
github-actions commentedon Jan 5, 2023
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 commentedon Jan 5, 2023
Was this confirmed as fixed as of #1216?
mhdawson commentedon Jan 5, 2023
I think so but @KevinEady can you confirm?
KevinEady commentedon Jan 5, 2023
Yep. Closing issue.
JosephusPaye commentedon Jan 7, 2023
Thank you both!