refactor: Skip manual signal disconnection in descructors #607

1. 
   hebasto commented at 12:30 pm on May 26, 2022: member

   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.

2. Skip manual signal disconnection in `~WalletModel()` dtor

   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

   596f43e786
3. Skip manual signal disconnection in `~TransactionTableModel()` dtor

   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

   6471fb5a73
4. Skip manual signal disconnection in `~BitcoinGUI()` dtor

   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

   611ced8714
5. Skip manual signal disconnection in `~ClientModel()` dtor

   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

   512072f9c9
6. hebasto added the label Refactoring on May 26, 2022
7. 
   promag commented at 12:50 pm on May 26, 2022: contributor

   Code review ACK 512072f9c969b414a71cec56e348ef6136a31385.
8. 
   DrahtBot commented at 0:02 am on May 27, 2022: contributor

   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.

9. furszy approved
10. 
    furszy commented at 5:28 pm on May 27, 2022: contributor

    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.

11. 
    jarolrod commented at 11:03 pm on May 28, 2022: member

    ACK 512072f9c969b414a71cec56e348ef6136a31385

    Confirmed PR description, and compiled and ran locally.

12. 
    hebasto commented at 2:40 pm on May 29, 2022: member

    @ryanofsky

    Would you mind having a look into this PR?

13. ryanofsky approved
14. 
    ryanofsky commented at 7:07 pm on May 31, 2022: contributor

    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.

15. 
    hebasto commented at 7:50 pm on May 31, 2022: member

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

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

16. 
    ryanofsky commented at 8:23 pm on May 31, 2022: contributor

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

17. 
    hebasto commented at 1:29 pm on July 19, 2022: member

    Thank all of reviewers for your reviews.

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

18. hebasto closed this on Jul 19, 2022

    ---

19. bitcoin-core locked this on Jul 19, 2023

Contributors
hebasto promag DrahtBot furszy jarolrod ryanofsky

Labels
Refactoring