Move scheduler/threadGroup into common-init instead of per-app #12266

1. 
   TheBlueMatt commented at 2:21 am on January 25, 2018: member

   This resolves #12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running.
2. fanquake added the label Refactoring on Jan 25, 2018
3. 
   TheBlueMatt commented at 2:23 am on January 25, 2018: member

   Needs an 0.16 tag.
4. TheBlueMatt force-pushed on Jan 25, 2018
5. sipa added this to the milestone 0.16.0 on Jan 25, 2018
6. in src/init.cpp:158 in 935f287a60 outdated

   154@@ -155,7 +155,10 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked
   155 static std::unique_ptr<CCoinsViewErrorCatcher> pcoinscatcher;
   156 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
   157 
   158-void Interrupt(boost::thread_group& threadGroup)
   159+boost::thread_group threadGroup;
   

   ---

   ---

   

   promag commented at 9:43 am on January 25, 2018:

   static?
7. in src/init.cpp:159 in 935f287a60 outdated

   154@@ -155,7 +155,10 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked
   155 static std::unique_ptr<CCoinsViewErrorCatcher> pcoinscatcher;
   156 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
   157 
   158-void Interrupt(boost::thread_group& threadGroup)
   159+boost::thread_group threadGroup;
   160+CScheduler scheduler;
   

   ---

   ---

   

   promag commented at 9:44 am on January 25, 2018:

   static?
8. 
   promag commented at 9:45 am on January 25, 2018: member

   Concept ACK.
9. Move scheduler/threadGroup into common-init instead of per-app

   This resolves #12229 which pointed out a shutdown deadlock due to
   scheduler/checkqueue having been shut down while network message
   processing is still running.

   082a61c69d
10. TheBlueMatt force-pushed on Jan 25, 2018
11. 
    theuni commented at 11:36 pm on January 26, 2018: member

    Since the scheduler is going to need a proper Interrupt/Shutdown before dropping the threadGroup anyway, can we just take this opportunity to create those? Then we could just skip the threadGroup.interrupt_all(), and the .join_all() would finish immediately.

    That would be much easier to review, IMO, because it makes the scheduler’s teardown procedure explicit.

12. 
    laanwj commented at 8:49 am on January 28, 2018: member

    Then we could just skip the threadGroup.interrupt_all(), and the .join_all() would finish immediately.

    Right - the longer term plan is to rid of ’threadgroup’ completely, by giving each ‘module’ its own Interrupt/Shutdown.

    But more often than not, changes to shutdown introduced new problems. I think this is very risky to do this last minute for 0.16.

    Is there a more minimalistic, easy to review fix for #12229 that we can do for 0.16, then leave the refactors for 0.17?

13. 
    TheBlueMatt commented at 2:19 pm on January 29, 2018: member

    Given that threadGroup is only used for scheduler and checkqueue, I’d strongly object to this being more than a rather-minimalistic change. The “obvious, minimalistic” change is to keep the scheduler running until after CConnman has been Stop()ed, which this does, the only additional thing is it keeps the checkqueue running, which is a rather trivial change.
14. 
    promag commented at 9:37 pm on January 29, 2018: member

    But more often than not, changes to shutdown introduced new problems. I think this is very risky to do this last minute for 0.16.

    Agree.

    Is there a way I can reproduce the issue?

15. 
    laanwj commented at 8:53 am on January 30, 2018: member

    I discussed with @TheBlueMatt on IRC and now I think this is very straightforward - there’s not much left using the thread group (only the scheduler and checkqueue), and anything that removes the global thread group will also need the changes to remove the arguments. So it’s a step forward in that regard.

    So utACK 082a61c69d7a539b5a48c4376a657f1c5aa92d81

16. 
    meshcollider commented at 11:09 am on January 30, 2018: contributor

    I’m not too familiar with the thread management but this looks straightforward enough for me to at least give a concept ACK / tentative utACK https://github.com/bitcoin/bitcoin/pull/12266/commits/082a61c69d7a539b5a48c4376a657f1c5aa92d81
17. laanwj merged this on Jan 30, 2018
18. laanwj closed this on Jan 30, 2018

    ---

19. laanwj referenced this in commit 3448907a68 on Jan 30, 2018
20. 
    morcos commented at 2:48 pm on January 30, 2018: member

    posthumous utACK
21. PastaPastaPasta referenced this in commit 3eff18c572 on Jan 17, 2020
22. PastaPastaPasta referenced this in commit 96e0837b3c on Jan 17, 2020
23. PastaPastaPasta referenced this in commit 3835feecd6 on Feb 12, 2020
24. PastaPastaPasta referenced this in commit 78a45b45df on Feb 12, 2020
25. codablock referenced this in commit bac693df27 on Mar 23, 2020
26. codablock referenced this in commit c01e39d610 on Mar 23, 2020
27. codablock referenced this in commit 4e8f4ea202 on Mar 24, 2020
28. ckti referenced this in commit 63adf464f0 on Mar 28, 2021
29. gades referenced this in commit 540432932e on Jun 30, 2021
30. DrahtBot locked this on Sep 8, 2021

Contributors
TheBlueMatt promag theuni laanwj meshcollider morcos

Labels
Refactoring

Milestone
0.16.0