Repository: bitcoin/bitcoin (language: cpp)

Issue:
Shutdown during reindex-chainstate can block forever
During `Shutdown`, we stop the scheduler before waiting on the load-block thread. But the load-block thread can call `LimitValidationInterfaceQueue` via `ActivateBestChain`. `LimitValidationInterfaceQueue` then schedules a dummy call and waits for it. But since the scheduler has stopped, it never gets there, and blocks forever. Shutdown remains joined to the thread, and also never exits.

Can we just wait for the load-block thread before killing the scheduler?


Discussion / hints:
I've observed such behavior but did not notice the reasons.

Thanks @luke-jr!
I had what I think was a similar issue (scriptcheck thread hanging, waiting on `SyncWithValidationInterfaceQueue` to complete). I found the changing `SyncWithValidationInterfaceQueue` to be:

```c++
void SyncWithValidationInterfaceQueue()
{
    AssertLockNotHeld(cs_main);
    // Block until the validation queue drains
    auto promise = std::make_shared<std::promise<void>>();
    CallFunctionInValidationInterfaceQueue([promise] {
        promise->set_value();
    });
    std::future_status status;
    do {
        status = promise->get_future().wait_for(10s);
    } while (status != std::future_status::ready); // && !ShutdownRequested());
}
```

fixed my problem. In particular, `Shutdown()` calls `scheduler->stop()` before `StopScriptCheckWorkerThreads()` which gives a deadlock: scheduler is stopped, but worker threads can't complete because they're waiting for the scheduler to finish off the promise. Just having the scriptcheck threads exit early isn't enough, because after the scriptcheck threads have finished `FlushBackgroundCallbacks()` is called which does the `promise->set_value()` above, so `promise` still needs to exist, hence making it a `shared_ptr`.
I also experienced this issue - my bitcoind hanged and when I dumped the running threads, it was waiting on `LimitValidationInterfaceQueue` but the scheduler thread had already exited. I did not have `reindex-chainstate` enabled. I think this can happen in any situation where the callbacks aren't cleared quickly enough and `SyncWithValidationInterfaceQueue` is called.

Fix patch:
diff --git a/src/init.cpp b/src/init.cpp
index e4b65fbfa94f6..03969b00bb661 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -296,10 +296,11 @@ void Shutdown(NodeContext& node)
 
     StopTorControl();
 
+    if (node.chainman && node.chainman->m_thread_load.joinable()) node.chainman->m_thread_load.join();
     // After everything has been shut down, but before things get flushed, stop the
-    // scheduler and load block thread.
+    // the scheduler. After this point, SyncWithValidationInterfaceQueue() should not be called anymore
+    // as this would prevent the shutdown from completing.
     if (node.scheduler) node.scheduler->stop();
-    if (node.chainman && node.chainman->m_thread_load.joinable()) node.chainman->m_thread_load.join();
 
     // After the threads that potentially access these pointers have been stopped,
     // destruct and reset all to nullptr.


Test patch: