shutdown: Stop threads before resetting ptrs #13894

1. 
   MarcoFalke commented at 4:13 pm on August 6, 2018: member

   On shutdown some threads would continue to run after or during a pointer reset. This leads to occasional segfaults on shutdown.

   Fix this by resetting the smart pointers after all threads that might read from them have been stopped.

   This should fix:

   • A segfault in the txindex thread, that occurs when the txindex destructor is done, but the thread was not yet stopped (as this is done in the base index destructor)
   • A segfault in the scheduler thread, which dereferences conman. (e.g. CheckForStaleTipAndEvictPeers)
2. MarcoFalke force-pushed on Aug 6, 2018
3. MarcoFalke force-pushed on Aug 6, 2018
4. 
   DrahtBot commented at 4:49 pm on August 6, 2018: member

   • #12934 ([net] [validation] Call ProcessNewBlock() asynchronously by skeees)

   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.

5. shutdown: Stop threads before resetting ptrs faab63111d
6. in src/init.cpp:222 in fa89c6339f outdated

   217@@ -222,6 +218,12 @@ void Shutdown()
   218     threadGroup.interrupt_all();
   219     threadGroup.join_all();
   220 
   221+    // After the threads that potentially access these pointers have been stopped,
   222+    // desctruct and reset all to nullptr.
   

   ---

   ---

   

   Empact commented at 7:52 pm on August 6, 2018:

   nit: destruct
7. MarcoFalke force-pushed on Aug 6, 2018
8. 
   MarcoFalke commented at 9:51 pm on August 6, 2018: member

   For reference on current master:

    02018-08-06T21:43:45Z Shutdown: In progress...
    12018-08-06T21:43:45Z dnsseed thread start
    22018-08-06T21:43:45Z net thread start
    32018-08-06T21:43:45Z msghand thread start
    42018-08-06T21:43:45Z addcon thread start
    52018-08-06T21:43:45Z opencon thread start
    62018-08-06T21:43:45Z addcon thread exit
    72018-08-06T21:43:45Z net thread exit
    82018-08-06T21:43:45Z torcontrol thread exit
    92018-08-06T21:43:45Z opencon thread exit
   102018-08-06T21:43:45Z dnsseed thread exit
   112018-08-06T21:43:45Z msghand thread exit
   12==14658== Thread 9 bitcoin-schedule:
   13==14658== Invalid read of size 8
   14==14658==    at 0x238DD4: PeerLogicValidation::CheckForStaleTipAndEvictPeers(Consensus::Params const&) (net_processing.cpp:3207)
   15
   16...
   17
   182018-08-06T21:43:47Z scheduler thread interrupt
   192018-08-06T21:43:47Z Dumped mempool: 0.00263s to copy, 0.017917s to dump
   202018-08-06T21:43:47Z Shutdown: done
   

   and

    02018-08-06T21:49:55Z Shutdown: In progress...
    12018-08-06T21:49:55Z txindex thread start
    2pure virtual method called
    3terminate called without an active exception
    4==14993== 
    5==14993== Process terminating with default action of signal 6 (SIGABRT): dumping core
    6==14993==    at 0x7115FEB: raise (in /usr/lib64/libc-2.27.so)
    7==14993==    by 0x71005C0: abort (in /usr/lib64/libc-2.27.so)
    8==14993==    by 0x660EA9A: ??? (in /usr/lib64/libstdc++.so.6.0.25)
    9==14993==    by 0x6614EFB: ??? (in /usr/lib64/libstdc++.so.6.0.25)
   10==14993==    by 0x6614F56: std::terminate() (in /usr/lib64/libstdc++.so.6.0.25)
   11==14993==    by 0x6615DA4: __cxa_pure_virtual (in /usr/lib64/libstdc++.so.6.0.25)
   12==14993==    by 0x387AF0: BaseIndex::ThreadSync() (base.cpp:140)
   13==14993==    by 0x1F4DBE: operator() (std_function.h:687)
   14==14993==    by 0x1F4DBE: void TraceThread<std::function<void ()> >(char const*, std::function<void ()>) (util.h:329)
   15==14993==    by 0x388BA6: __invoke_impl<void, void (*)(char const*, std::function<void()>), char const*, std::_Bind<void (BaseIndex::*(BaseIndex*))()> > (invoke.h:60)
   16==14993==    by 0x388BA6: __invoke<void (*)(char const*, std::function<void()>), char const*, std::_Bind<void (BaseIndex::*(BaseIndex*))()> > (invoke.h:95)
   17==14993==    by 0x388BA6: _M_invoke<0, 1, 2> (thread:234)
   18==14993==    by 0x388BA6: operator() (thread:243)
   19==14993==    by 0x388BA6: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void ()>), char const*, std::_Bind<void (BaseIndex::*(BaseIndex*))()> > > >::_M_run() (thread:186)
   20==14993==    by 0x6641522: ??? (in /usr/lib64/libstdc++.so.6.0.25)
   21==14993==    by 0x6EC7593: start_thread (in /usr/lib64/libpthread-2.27.so)
   22==14993==    by 0x71D90DE: clone (in /usr/lib64/libc-2.27.so)
   23==14993== 
   24==14993== HEAP SUMMARY:
   25==14993==     in use at exit: 59,700,169 bytes in 84,309 blocks
   26==14993==   total heap usage: 98,728 allocs, 14,419 frees, 88,664,198 bytes allocated
   27==14993== 
   28==14993== LEAK SUMMARY:
   29==14993==    definitely lost: 9,085,501 bytes in 81,256 blocks
   30==14993==    indirectly lost: 856 bytes in 8 blocks
   31==14993==      possibly lost: 13,139,949 bytes in 33 blocks
   32==14993==    still reachable: 37,473,863 bytes in 3,012 blocks
   33==14993==                       of which reachable via heuristic:
   34==14993==                         newarray           : 48 bytes in 1 blocks
   35==14993==         suppressed: 0 bytes in 0 blocks
   36==14993== Rerun with --leak-check=full to see details of leaked memory
   37==14993== 
   38==14993== For counts of detected and suppressed errors, rerun with: -v
   39==14993== Use --track-origins=yes to see where uninitialised values come from
   40==14993== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
   41Aborted (core dumped)
   

9. in src/init.cpp:212 in faab63111d

   208@@ -209,11 +209,7 @@ void Shutdown()
   209     // using the other before destroying them.
   210     if (peerLogic) UnregisterValidationInterface(peerLogic.get());
   211     if (g_connman) g_connman->Stop();
   212-    peerLogic.reset();
   213-    g_connman.reset();
   214-    if (g_txindex) {
   215-        g_txindex.reset();
   216-    }
   217+    if (g_txindex) g_txindex->Stop();
   

   ---

   ---

   

   domob1812 commented at 7:37 am on August 7, 2018:

   Since this is newly added, was it missing completely (independent of the position of pointer resets)?

   ---

   

   MarcoFalke commented at 10:31 am on August 7, 2018:

   The base destructor was calling it, but yes, imo it was missing.

   ---

   

   domob1812 commented at 12:26 pm on August 7, 2018:

   Thanks for fixing then especially!

   ---

   

   MarcoFalke commented at 3:46 pm on August 7, 2018:

   To clarify: right now it would be called twice. Once here and then another time (as belt-and-supenders) in the destructor when the pointer is reset. (Just like for conman)

   Though, the belt-and-suspenders are not enough and can lead to segfaults occasionally. (See above valgrind output)

10. 
    domob1812 commented at 7:37 am on August 7, 2018: contributor

    utACK faab63111d8f27335aa1f09c1a48da3be45135de
11. laanwj added the label Bug on Aug 7, 2018
12. laanwj added this to the milestone 0.17.0 on Aug 7, 2018
13. 
    achow101 commented at 0:45 am on August 8, 2018: member

    utACK faab63111d8f27335aa1f09c1a48da3be45135de
14. ken2812221 referenced this in commit df9f712746 on Aug 8, 2018
15. laanwj merged this on Aug 8, 2018
16. laanwj closed this on Aug 8, 2018

    ---

17. 
    laanwj commented at 1:21 pm on August 8, 2018: member

    utACK faab63111d8f27335aa1f09c1a48da3be45135de
18. MarcoFalke deleted the branch on Aug 8, 2018
19. MarcoFalke referenced this in commit e8061831e8 on Aug 27, 2018
20. codablock referenced this in commit 35b041ed14 on Mar 23, 2020
21. codablock referenced this in commit 4e8f4ea202 on Mar 24, 2020
22. random-zebra referenced this in commit ac523660b4 on Feb 21, 2021
23. ckti referenced this in commit 88860b3fce on Mar 28, 2021
24. PastaPastaPasta referenced this in commit 0f3eb52994 on Jun 27, 2021
25. PastaPastaPasta referenced this in commit 00d2dbdfc1 on Jun 28, 2021
26. PastaPastaPasta referenced this in commit bebc5867b9 on Jun 29, 2021
27. PastaPastaPasta referenced this in commit 973195435c on Jun 29, 2021
28. PastaPastaPasta referenced this in commit c4987e742a on Jun 29, 2021
29. PastaPastaPasta referenced this in commit f61686093d on Jun 29, 2021
30. PastaPastaPasta referenced this in commit 289873d39e on Jun 29, 2021
31. Munkybooty referenced this in commit bd0763196a on Jun 30, 2021
32. Munkybooty referenced this in commit d74b916e77 on Jun 30, 2021
33. Munkybooty referenced this in commit c7fb300024 on Jul 1, 2021
34. PastaPastaPasta referenced this in commit 6bf1fbbf09 on Jul 1, 2021
35. DrahtBot locked this on Sep 8, 2021

Contributors
MarcoFalke DrahtBot Empact domob1812 achow101 laanwj

Labels
Bug

Milestone
0.17.0