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

fanquake · 2020-06-07T05:37:25Z

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

hebasto · 2020-06-07T06:18:40Z

Concept ACK.

hebasto

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

hebasto · 2020-06-07T06:55:01Z

src/init.cpp

+    std::thread import(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); });
+    import.detach();


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

hebasto · 2020-06-07T07:12:16Z

Keep testing and the second instance of "loadblk" thread appears:

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.

maflcko · 2020-06-07T10:26:32Z

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

sipa · 2020-06-07T16:28:22Z

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

maflcko · 2020-06-07T16:34:31Z

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 · 2020-06-07T16:57:38Z

@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?

#include <thread>
#include <chrono>
#include <iostream>

using namespace std::chrono_literals;
struct Destruct
{
    ~Destruct() { std::cout << "Dtor called" << std::endl; }
};

void ThreadProc()
{
  Destruct d;
  std::this_thread::sleep_for(1s);
}

int main(int argc, char* argv[])
{
  std::cout << "Hello" << std::endl;

  std::thread t(ThreadProc);
  //t.join();
  t.detach();

  std::cout << "Bye" << std::endl;
  return 0;
}

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 · 2020-06-07T17:05:38Z

@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 · 2020-06-09T14:35:58Z

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 · 2020-06-12T07:47:31Z

Pushed a .detach()-less change.

src/init.cpp

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

donaloconnor · 2020-06-12T08:08:28Z

ACK 83fd3a6

maflcko · 2020-06-12T11:22:45Z

src/init.cpp

@@ -152,6 +152,8 @@ NODISCARD static bool CreatePidFile()

 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;

+static std::thread g_load_block;


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?

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.

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

diff --git a/src/init.cpp b/src/init.cpp index 8d9566edc3..984f6b2034 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -152,8 +152,6 @@ NODISCARD static bool CreatePidFile() static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle; -static std::thread g_load_block; - static boost::thread_group threadGroup; void Interrupt(NodeContext& node) @@ -218,7 +216,7 @@ void Shutdown(NodeContext& node) // After everything has been shut down, but before things get flushed, stop the // CScheduler/checkqueue, threadGroup and load block thread. if (node.scheduler) node.scheduler->stop(); - if (g_load_block.joinable()) g_load_block.join(); + if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join(); threadGroup.interrupt_all(); threadGroup.join_all(); @@ -1844,7 +1842,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node) vImportFiles.push_back(strFile); } - g_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); }); + chainman.m_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); }); // Wait for genesis block to be processed { diff --git a/src/validation.h b/src/validation.h index e403bcb51a..9764a7aaca 100644 --- a/src/validation.h +++ b/src/validation.h @@ -783,6 +783,7 @@ private: friend CChain& ChainActive(); public: + std::thread m_load_block; //! A single BlockManager instance is shared across each constructed //! chainstate to avoid duplicating block metadata. BlockManager m_blockman GUARDED_BY(::cs_main);

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 · 2020-06-16T15:34:15Z

Code review ACK 83fd3a6

maflcko

ACK 83fd3a6

maflcko · 2020-06-16T16:22:11Z

src/init.cpp

@@ -152,6 +152,8 @@ NODISCARD static bool CreatePidFile()

 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;

+static std::thread g_load_block;


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

diff --git a/src/init.cpp b/src/init.cpp index 8d9566edc3..984f6b2034 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -152,8 +152,6 @@ NODISCARD static bool CreatePidFile() static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle; -static std::thread g_load_block; - static boost::thread_group threadGroup; void Interrupt(NodeContext& node) @@ -218,7 +216,7 @@ void Shutdown(NodeContext& node) // After everything has been shut down, but before things get flushed, stop the // CScheduler/checkqueue, threadGroup and load block thread. if (node.scheduler) node.scheduler->stop(); - if (g_load_block.joinable()) g_load_block.join(); + if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join(); threadGroup.interrupt_all(); threadGroup.join_all(); @@ -1844,7 +1842,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node) vImportFiles.push_back(strFile); } - g_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); }); + chainman.m_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); }); // Wait for genesis block to be processed { diff --git a/src/validation.h b/src/validation.h index e403bcb51a..9764a7aaca 100644 --- a/src/validation.h +++ b/src/validation.h @@ -783,6 +783,7 @@ private: friend CChain& ChainActive(); public: + std::thread m_load_block; //! A single BlockManager instance is shared across each constructed //! chainstate to avoid duplicating block metadata. BlockManager m_blockman GUARDED_BY(::cs_main);

hebasto

ACK 83fd3a6, 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.

83fd3a6 init: use std::thread for ThreadImport() (fanquake) Pull request description: [Mentioned](bitcoin#19142 (comment)) in bitcoin#19142, which removed the `boost::interruption_point()` in `ThreadImport()`. ACKs for top commit: hebasto: ACK 83fd3a6, I have reviewed the code and it looks OK, I agree it can be merged. donaloconnor: ACK 83fd3a6 laanwj: Code review ACK 83fd3a6 MarcoFalke: ACK 83fd3a6 Tree-SHA512: 0644947d669feb61eed3a944012dad1bd3dd75cf994aa2630013043c213a335b162b63e20aa37e0997740d8e3a3ec367b660b5196007a09e13f0ac455b36c821

This is a diff of Marcos from bitcoin#19197, which probably should have just been used at the time. After this change, and bitcoin#21016, we'll have no more global thread(Group)s hanging out in init. Co-authored-by: MarcoFalke <falke.marco@gmail.com>

Summary: Mentioned in [[bitcoin/bitcoin#19142 | core#19142]], which removed the boost::interruption_point() in ThreadImport(). This is a backport of [[bitcoin/bitcoin#19197 | core#19197]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9833

fanquake added the Refactoring label Jun 7, 2020

hebasto approved these changes Jun 7, 2020

View reviewed changes

hebasto mentioned this pull request Jun 7, 2020

Unidentified thread created in FlushStateToDisk() #19199

Closed

fanquake force-pushed the thread_import_no_boost branch from aeb100f to 2aca26d Compare June 12, 2020 07:42

donaloconnor reviewed Jun 12, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

init: use std::thread for ThreadImport()

83fd3a6

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

fanquake force-pushed the thread_import_no_boost branch from 2aca26d to 83fd3a6 Compare June 12, 2020 08:05

maflcko reviewed Jun 12, 2020

View reviewed changes

maflcko reviewed Jun 16, 2020

View reviewed changes

hebasto approved these changes Jun 18, 2020

View reviewed changes

fanquake merged commit 057bd31 into bitcoin:master Jun 19, 2020

fanquake deleted the thread_import_no_boost branch June 19, 2020 05:29

hebasto mentioned this pull request Jul 3, 2020

refactor: Cleanup thread ctor calls #19064

Merged

fanquake mentioned this pull request Jan 29, 2021

refactor: move load block thread into ChainstateManager #21030

Closed

str4d mentioned this pull request Dec 6, 2021

Backport upstream PRs that remove Boost usage zcash/zcash#4819

Open

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		std::thread import(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); });
		import.detach();

		@@ -152,6 +152,8 @@ NODISCARD static bool CreatePidFile()

		static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;

		static std::thread g_load_block;

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

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

Uh oh!

Conversation

fanquake commented Jun 7, 2020

Uh oh!

hebasto commented Jun 7, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 7, 2020

Uh oh!

sipa commented Jun 7, 2020

Uh oh!

maflcko commented Jun 7, 2020

Uh oh!

donaloconnor commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 7, 2020

Uh oh!

laanwj commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 12, 2020

Uh oh!

Uh oh!

donaloconnor commented Jun 12, 2020

Uh oh!

maflcko Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

fanquake Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 16, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto Jun 7, 2020 •

edited

Loading

hebasto commented Jun 7, 2020 •

edited

Loading

donaloconnor commented Jun 7, 2020 •

edited

Loading

laanwj commented Jun 9, 2020 •

edited

Loading

laanwj Jun 16, 2020 •

edited

Loading