init: use std::thread for ThreadImport()

fanquake commented at 5:37 am on June 7, 2020: member

Mentioned in #19142, which removed the boost::interruption_point() in ThreadImport().

fanquake added the label Refactoring on Jun 7, 2020

hebasto commented at 6:18 am on June 7, 2020: member

Concept ACK.

in src/init.cpp:1856 in aeb100f79e outdated

1851@@ -1853,7 +1852,8 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
1852         vImportFiles.push_back(strFile);
1853     }
1854 
1855-    threadGroup.create_thread([=, &chainman] { ThreadImport(chainman, vImportFiles); });
1856+    std::thread import(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); });
1857+    import.detach();

hebasto commented at 6:55 am on June 7, 2020:

nit: The AppInitMain() is a way large function. Could the scope of the import variable be limited by a block?

hebasto approved

hebasto commented at 6:59 am on June 7, 2020: member

ACK aeb100f79ec6ff816ee26344b4eef70001dd51a6, tested on Linux Mint 19.3 (x86_64) with -reindex command-line option and thread monitoring via htop.

hebasto commented at 7:12 am on June 7, 2020: member

Keep testing and the second instance of “loadblk” thread appears:

Screenshot from 2020-06-07 10-02-35

Is it ok?

UPDATE: the same behavior observed on master, therefore it seems does not related to this PR changes. Going to submit a dedicated issue.

MarcoFalke commented at 10:26 am on June 7, 2020: member

why is is ok to not wait for the thread to exit (join) before shutdown?

sipa commented at 4:28 pm on June 7, 2020: member

@MarcoFalke AFAIK, there is no problem with that. The application won’t quit until all its threads have terminated.

MarcoFalke commented at 4:34 pm on June 7, 2020: member

What I mean is that during shutdown we assume the chain clients don’t receive (let’s say) block connected signals anymore because they are already flushed. After this change, it is (theoretically, I haven’t checked) possible that ThreadImport generates events that will be sent to objects that can’t handle them or that ThreadImport reads from objects that have been deleted.

Am I missing something obvious?

donaloconnor commented at 4:57 pm on June 7, 2020: contributor

@MarcoFalke AFAIK, there is no problem with that. The application won’t quit until all its threads have terminated.

I thought that once main returns, any detached threads will not be waited on and stack unwinding does not occur risking danger of uncalled dtors.

I tried a sample here and it seems to be the case?

 0#include <thread>
 1#include <chrono>
 2#include <iostream>
 3
 4using namespace std::chrono_literals;
 5struct Destruct
 6{
 7    ~Destruct() { std::cout << "Dtor called" << std::endl; }
 8};
 9
10void ThreadProc()
11{
12  Destruct d;
13  std::this_thread::sleep_for(1s);
14}
15
16int main(int argc, char* argv[])
17{
18  std::cout << "Hello" << std::endl;
19
20  std::thread t(ThreadProc);
21  //t.join();
22  t.detach();
23
24  std::cout << "Bye" << std::endl;
25  return 0;
26}

Output is Hello,Bye instead of Hello,Dtor called,Bye. Edit: above output wouldn’t happen anyway. - but I’m wondering if dtor really gets called or not. I’ll do more research.

sipa commented at 5:05 pm on June 7, 2020: member

@donaloconnor I would certainly have expected the code above to sleep 1 s before exiting, so it does seem I’m wrong. @MarcoFalke That’s a good point. It seems we’ll need to keep the std::thread object around somehow, and join it in the right order at shutdown.

laanwj commented at 2:35 pm on June 9, 2020: member

Concept ACK.

@MarcoFalke That’s a good point. It seems we’ll need to keep the std::thread object around somehow, and join it in the right order at shutdown.

Yes. It needs to be join-ed on shutdown to make sure it has terminated before tearing down validation-related data structures. Detach is not safe for a thread like this.

(there’s only one use where using detach is acceptable, that is the treads spawning child processes for notification that run free)

fanquake force-pushed on Jun 12, 2020

fanquake commented at 7:47 am on June 12, 2020: member

Pushed a .detach()-less change.

in src/init.cpp:221 in 2aca26d5dd outdated

215@@ -214,8 +216,9 @@ void Shutdown(NodeContext& node)
216     StopTorControl();
217 
218     // After everything has been shut down, but before things get flushed, stop the
219-    // CScheduler/checkqueue threadGroup
220+    // CScheduler/checkqueue, threadGroup and load block thread.
221     if (node.scheduler) node.scheduler->stop();
222+    g_load_block.join();

donaloconnor commented at 7:59 am on June 12, 2020:

I think it might be safer to check if the thread is joinable before joining in case this path is reached after some initialization failure when the thread may not have started.

fanquake commented at 8:06 am on June 12, 2020:

Thanks, I’ve added that.

init: use std::thread for ThreadImport()

Mentioned in #19142, which removed the boost::interruption_point()
in ThreadImport().

83fd3a6d73

fanquake force-pushed on Jun 12, 2020

donaloconnor commented at 8:08 am on June 12, 2020: contributor

ACK 83fd3a6

in src/init.cpp:155 in 83fd3a6d73

151@@ -152,6 +152,8 @@ NODISCARD static bool CreatePidFile()
152 
153 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
154 
155+static std::thread g_load_block;

MarcoFalke commented at 11:22 am on June 12, 2020:

I don’t really like adding more and more globals to init. Ideally init should be a slim file with only calls to other modules to start/stop. Longer term ThreadImport should be moved to validation because it is purely validation related. Init shouldn’t need to know how to load blocks from disk, etc… Note that LoadMempool is also in validation.

Moreover, with assumeutxo, thus multiple chainstates, I could imagine that a reindex/import can be done in the background chainstate for additional efficiency. Having init to deal with even more chainstate related logic would feel wrong.

What do you think about making this thread a member of the chainstate manager?

laanwj commented at 3:33 pm on June 16, 2020:

On the other hand this was a hidden global before, a part of threadGroup (which can hopefully go away soon!). I don’t think this PR makes anything worse.

I think your longer-term solution makes sense, but I’m not sure it needs to be done here.

MarcoFalke commented at 4:22 pm on June 16, 2020:

Agree that it doesn’t make anything worse, but it could be slightly better with a trivial diff.

 0diff --git a/src/init.cpp b/src/init.cpp
 1index 8d9566edc3..984f6b2034 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -152,8 +152,6 @@ NODISCARD static bool CreatePidFile()
 5 
 6 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
 7 
 8-static std::thread g_load_block;
 9-
10 static boost::thread_group threadGroup;
11 
12 void Interrupt(NodeContext& node)
13@@ -218,7 +216,7 @@ void Shutdown(NodeContext& node)
14     // After everything has been shut down, but before things get flushed, stop the
15     // CScheduler/checkqueue, threadGroup and load block thread.
16     if (node.scheduler) node.scheduler->stop();
17-    if (g_load_block.joinable()) g_load_block.join();
18+    if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join();
19     threadGroup.interrupt_all();
20     threadGroup.join_all();
21 
22@@ -1844,7 +1842,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
23         vImportFiles.push_back(strFile);
24     }
25 
26-    g_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); });
27+    chainman.m_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); });
28 
29     // Wait for genesis block to be processed
30     {
31diff --git a/src/validation.h b/src/validation.h
32index e403bcb51a..9764a7aaca 100644
33--- a/src/validation.h
34+++ b/src/validation.h
35@@ -783,6 +783,7 @@ private:
36     friend CChain& ChainActive();
37 
38 public:
39+    std::thread m_load_block;
40     //! A single BlockManager instance is shared across each constructed
41     //! chainstate to avoid duplicating block metadata.
42     BlockManager m_blockman GUARDED_BY(::cs_main);

fanquake commented at 5:26 am on June 19, 2020:

Thanks. I’ll take care of the suggestions, adding to chainman and thread renaming, in some followup changes. I just wanted to get it out of the Boost thread group here.

laanwj commented at 3:34 pm on June 16, 2020: member

Code review ACK 83fd3a6d73eee452dc5141bdf6826da62d7b2dbd

MarcoFalke commented at 4:22 pm on June 16, 2020: member

ACK 83fd3a6d73eee452dc5141bdf6826da62d7b2dbd

hebasto approved

hebasto commented at 12:15 pm on June 18, 2020: member

ACK 83fd3a6d73eee452dc5141bdf6826da62d7b2dbd, I have reviewed the code and it looks OK, I agree it can be merged.

While this code is touched already, the “loadblk” thread could be renamed to “loadblocks” (10 characters from 13 allowed) or “importblocks” (12 characters) for readability.

fanquake merged this on Jun 19, 2020

fanquake closed this on Jun 19, 2020

fanquake deleted the branch on Jun 19, 2020

sidhujag referenced this in commit 3598bbfdba on Jul 7, 2020

fanquake referenced this in commit f538c25cd4 on Jan 29, 2021

fanquake referenced this in commit be7065623a on Feb 2, 2021

fanquake referenced this in commit e3341dfaf7 on Feb 2, 2021

MarcoFalke referenced this in commit 63984b38f0 on Apr 2, 2021

deadalnix referenced this in commit ae09ca09e4 on Jul 23, 2021

DrahtBot locked this on Feb 15, 2022

init: use std::thread for ThreadImport() #19197