refactor: Skip manual signal disconnection in descructors

[hebasto]

Internally, interfaces::HandlerImpl keeps a member of the boost::signals2::scoped_connection type

gui/src/interfaces/handler.cpp

Line 21 in 192d639

boost::signals2::scoped_connection m_connection;

which automatically disconnects on destruction.

Therefore, manual disconnections are redundant in class destructors.

[promag]

Code review ACK 512072f.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #417 (Ditch wallet model juggling by promag)

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.

[furszy]

Code review ACK 512072f

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.

[jarolrod]

ACK 512072f

Confirmed PR description, and compiled and ran locally.

[hebasto]

@ryanofsky

Would you mind having a look into this PR?

[ryanofsky]

Tentative code review ACK 512072f. 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.

[hebasto]

... now it could call methods on a half destroyed object.

During destruction, only one thread has access to the object, right?

[ryanofsky]

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

[hebasto]

Thank all of reviewers for your reviews.

Going to close this PR to do not raise safety concerns.