Internally, interfaces::HandlerImpl keeps a member of the boost::signals2::scoped_connection type https://github.com/bitcoin-core/gui/blob/192d639a6b1bd0feaa52e6ea4e63e33982704c32/src/interfaces/handler.cpp#L21 which automatically disconnects on destruction.
Therefore, manual disconnections are redundant in class destructors.
A boost::signals2::scoped_connection automatically disconnects on
its destruction.
https://www.boost.org/doc/libs/1_64_0/doc/html/boost/signals2/scoped_connection.html
A boost::signals2::scoped_connection automatically disconnects on
its destruction.
https://www.boost.org/doc/libs/1_64_0/doc/html/boost/signals2/scoped_connection.html
A boost::signals2::scoped_connection automatically disconnects on
its destruction.
https://www.boost.org/doc/libs/1_64_0/doc/html/boost/signals2/scoped_connection.html
A boost::signals2::scoped_connection automatically disconnects on
its destruction.
https://www.boost.org/doc/libs/1_64_0/doc/html/boost/signals2/scoped_connection.html
Code review ACK 512072f9c969b414a71cec56e348ef6136a31385.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Code review ACK 512072f9
To not forget about this, would be nice to add a doc comment at the top of the HandlerImpl class and MakeHandler(boost::signals2::connection) explaining that the returned Handler disconnects the signal when the object is destructed.
ACK 512072f9c969b414a71cec56e348ef6136a31385
Confirmed PR description, and compiled and ran locally.
Would you mind having a look into this PR?
Tentative code review ACK 512072f9c969b414a71cec56e348ef6136a31385. For all the commits except the first commit, the destructors are non-empty, so this PR changes behavior by disconnecting signals after the other code in the destructor instead of before the other code in the constructor. In theory this could be a problem if a new signal is sent while the object is being destroyed. Previously the signal would be ignored, now it could call methods on a half destroyed object.
It would be good if commit messages acknowledged the delayed disconnection of signals, and perhaps explained why it was safe to delay disconnecting signals.
... now it could call methods on a half destroyed object.
During destruction, only one thread has access to the object, right?
... now it could call methods on a half destroyed object.
During destruction, only one thread has access to the object, right?
The lambdas passed to handleNotification methods are capturing [this] so they can reference the object when notifications are sent. That is why it might be important to disconnect the notifications earlier instead of later. But I don't know whether this is important, which is why I raised the question. Also even if notifications were single threaded (I don't think they are) it might still matter if they are disconnected later during destruction instead of earlier, because maybe later code in the destructor could trigger them.
Overall, I would guess this PR is probably safe, I just think it could use a better explanation of how it is safe.
Thank all of reviewers for your reviews.
Going to close this PR to do not raise safety concerns.