Nuke boost::thread and boost::thread_group #8631

theuni · 2016-08-31T02:33:56Z

This replaces #8023 with a solution that moves us directly to std::thread, rather than using an intermediate wrapper.

Background

std::thread is pretty much a drop-in replacement for boost::thread, except that boost::thread is interruptible. We've used those interruptions, but we'll have to drop them eventually as they didn't become part of the spec.

When an interruption point is hit, it throws an instance of boost::thread_interrupted. boost::thread catches these automatically by default.

The interruption points are defined as:


    boost::thread::join()
    boost::thread::timed_join()
    boost::thread::try_join_for(),
    boost::thread::try_join_until(),
    boost::condition_variable::wait()
    boost::condition_variable::timed_wait()
    boost::condition_variable::wait_for()
    boost::condition_variable::wait_until()
    boost::condition_variable_any::wait()
    boost::condition_variable_any::timed_wait()
    boost::condition_variable_any::wait_for()
    boost::condition_variable_any::wait_until()
    boost::thread::sleep()
    boost::this_thread::sleep_for()
    boost::this_thread::sleep_until()
    boost::this_thread::interruption_point()

The ones relevant to us are primarily boost::condition_variable_any::wait(), boost::this_thread::sleep_for(), and of course boost::this_thread::interruption_point()

Any boost::thread will immediately throw boost::thread_interrupted if those are hit at any point. So we can't simply s/boost::thread/std::thread/g, as we would never be able to exit.

Changes

All threads are now externally interruptible. Rather than having a single bool for shutting down, I've attempted to notify each relevant subsystem. But because they're not well-defined, the notifications are somewhat arbitrary as well.
For long-running threads, our own thread_interrupted and interruption_point have been added.
Threads should call TraceThread if they may hit an interruption_point.
MilliSleep is no longer interruptible.
boost::thread_group is gone. All threads are now managed individually

Future changes

There is lots of cleanup to be done post-merge. I was very tempted to include cleanups here, but I've elected to keep the changes minimal. Todo:

s/boost::mutex/std::mutex/
s/boost::condition_variable/std::condition_variable/
s/boost::chrono/std::chrono/
Drop a ton of boost includes
Drop other boost cruft (like catching boost::thread_interrupted)

Also, I started on this by creating a base class for launching threads. Each instance would be responsible for implementing Interrupt() and Stop(). But because this is already a complex set of changes, I think it's best to do that as a separate step.

JeremyRubin · 2016-08-31T03:46:44Z

Concept ACK.

Will review & test soon.

The boost interruption_point paradigm is messy and can make code hard to reason about. In my code for #8464 I have also removed boost::thread and interruption points, preferring more explicit quitting. Dealing with the interruption code made working on #8464 more difficult than it had to be.

One Nit, which I think is worth addressing. I would refactor the names from Interrupt to Quit, Shutdown, or even ControlledExit. Connotation of interrupt is a non-consensual break, whereas Quit or Shutdown implies being done as a normal part of control flow.

theuni · 2016-08-31T03:52:18Z

@JeremyRubin Nearly every thread uses separate interrupt/shutdown calls for exactly that reason.

Interrupt is non-consensual, and means "stop what you're doing and don't start anything else". Shutdown means "please gracefully exit now and don't return until you do".

JeremyRubin · 2016-08-31T03:59:05Z

@theuni Ah, gotcha. Was looking at small sample; my bad. I guess my comment should instead be, we should eventually migrate to shutdown only no interrupt threads; but this is a big step in that direction and precursor to that happening.

sipa · 2016-08-31T15:30:53Z

Concept ACK, testing.

paveljanik · 2016-08-31T16:02:15Z

OS X here:

Making all in src
  CXX      test/test_test_bitcoin-scheduler_tests.o
In file included from test/scheduler_tests.cpp:8:
In file included from ./test/test_bitcoin.h:16:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/thread:337:5: error: attempt to use a deleted function
    __invoke(std::__1::move(std::__1::get<0>(__t)), std::__1::move(std::__1::get<_Indices>(__t))...);
    ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/thread:347:5: note: in instantiation of function template specialization 'std::__1::__thread_execute<void (CScheduler::*)(), std::__1::reference_wrapper<CScheduler> , 1>' requested here
    __thread_execute(*__p, _Index());
    ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/thread:359:42: note: in instantiation of function template specialization 'std::__1::__thread_proxy<std::__1::tuple<void (CScheduler::*)(), std::__1::reference_wrapper<CScheduler> > >' requested here
    int __ec = pthread_create(&__t_, 0, &__thread_proxy<_Gp>, __p.get());
                                         ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1673:31: note: in instantiation of function template specialization 'std::__1::thread::thread<void (CScheduler::*)(), std::__1::reference_wrapper<CScheduler> , void>' requested here
            ::new((void*)__p) _Up(std::__1::forward<_Args>(__args)...);
                              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1600:18: note: in instantiation of function template specialization 'std::__1::allocator<std::__1::thread>::construct<std::__1::thread, void (CScheduler::*)(), std::__1::reference_wrapper<CScheduler> >' requested here
            {__a.construct(__p, std::__1::forward<_Args>(__args)...);}
                 ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1453:14: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<std::__1::thread> >::__construct<std::__1::thread, void (CScheduler::*)(), std::__1::reference_wrapper<CScheduler> >' requested here
            {__construct(__has_construct<allocator_type, _Tp*, _Args...>(),
             ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:1643:25: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<std::__1::thread> >::construct<std::__1::thread, void (CScheduler::*)(), std::__1::reference_wrapper<CScheduler> >' requested here
        __alloc_traits::construct(this->__alloc(),
                        ^
test/scheduler_tests.cpp:89:22: note: in instantiation of function template specialization 'std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread> >::emplace_back<void (CScheduler::*)(), std::__1::reference_wrapper<CScheduler> >' requested here
        microThreads.emplace_back(&CScheduler::serviceQueue, std::ref(microTasks));
                     ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/type_traits:1043:5: note: '~__nat' has been explicitly marked deleted here
    ~__nat() = delete;
    ^

paveljanik · 2016-08-31T18:04:30Z

Compile error fixed here.

sipa · 2016-08-31T23:06:39Z

Needs rebase.

theuni · 2016-09-01T00:10:50Z

rebased and squashed in the compile fix.

laanwj · 2016-09-01T12:10:58Z

Nearly every thread uses separate interrupt/shutdown calls for exactly that reason.

There's another reason, and that is that interrupt is supposed to be an instant operation at the caller site, and shutdown is allowed to wait. This allows parallelism in the shutdown process:

Send module A interrupt (A starts terminating its threads, which may take some time)
Send module B interrupt (B starts terminating its threads, which may take some time)

...

Module A shutdown (waits for A's threads to join)
Module B shutdown (waits for B's threads to join)

Sure, the naming may not be optimal, "interrupt" is more like a hint to stop work, but I think the approach makes sense.

(I'm sure this can be implemented more thoroughly with explicit shutdown-dependencies and std::futures, but the idea is the same, to make shutdown as fast as possible)

Edit: re-triggered travis, the failure was a timeout, probably has nothing to do with the changes here.

laanwj · 2016-09-01T13:47:03Z

Travis does keep failing. There may be an issue after all.

theuni · 2016-09-01T15:32:03Z

Hmm, travis was happy before the rebase. I'll try to reproduce locally, maybe something changed.

theuni · 2016-09-02T05:05:28Z

The travis failures were caused by the rebase after all. ab48c5e introduced a new test that relied on the interruptible semantics. I've added a commit that should fix that up.

btcdrak · 2016-09-02T11:21:24Z

needs rebase again :(

Also note that boost::thread catches boost::thread_interrupted itself, but std::thread obviously won't catch thread_interrupted. So don't rethrow after catching.

Also, drop threadGroup for them and treat them individually

…p_for

JeremyRubin · 2016-09-05T07:58:20Z

src/net.cpp

@@ -120,6 +120,12 @@ boost::condition_variable messageHandlerCondition;
 static CNodeSignals g_signals;
 CNodeSignals& GetNodeSignals() { return g_signals; }

+static std::condition_variable net_interrupt_cond;
+static std::mutex cs_net_interrupt;
+static std::atomic<bool> net_interrupted(false);


This is OK -- initially, I was confused as to why you use an atomic var for the condition variable, as usually access is guarded by the lock so atomic would be overkill. But in this case, it's fine.

May be worth writing a profile at some point for if it is better to use a cond+atomic or to lock on writing the condition externally, or just throwing a comment in here as to why you're using an atomic.

Right, that's not at all clear from the individual commits. The atomic is used as the break condition for a few condvars, as well as the loop break condition in a few places. Rather than using separate bools for each, I just lumped them all into this one.

Will add a comment.

JeremyRubin · 2016-09-05T08:57:42Z

@theuni I've gone through and I'll give you a "tentative" utack. Mostly looks fine.

I'd suggest mostly:

checking typedefs which secretly use boost::*
Getting rid of the exception throwing interruption point. It's a bad default.
Getting rid of std::bind in favor of lambdas.
depending on status of my wip scriptcheck queue stuff, leaving those two commits out.

theuni · 2016-09-06T19:36:29Z

@JeremyRubin Thanks for the review! To your points:

I agree somewhat on the typedefs, but I'd like to avoid this PR getting bigger and bigger. So let's do these case-by-case.
I don't think we can get rid of the exception throwing yet. I hate the interruption_point logic, but I don't think it's realistic to remove it all in one PR. I certainly agree with minimizing it though.
Agree on losing the binds where possible
Without the scriptcheck changes, this can't be tested as we can't shutdown. So I'm afraid we'll have to step on eachother's toes until one is merged. Obviously I'm happy to drop the scriptcheck changes and adapt to the new stuff if it goes in first.

JeremyRubin · 2016-09-06T19:57:03Z

@theuni The only one I think you need to fix for this pr is CConditionVariable. Were there others you're worried about?

No worries about the scriptcheck stuff, we'll cross that bridge when it comes.

theuni · 2016-09-06T21:01:28Z

@JeremyRubin Fixing up CConditionVariable means doing lots of mutex/lock replacements all over the place. I'd really rather do that as a follow-up to keep this one minimal.

Now that we're not using any boost::thread's, I don't believe there's any major functional difference.

JeremyRubin · 2016-09-06T21:22:59Z

@theuni sounds fine, this one is nuking boost::thread not boost::condition_variable after all :)

btcdrak · 2016-09-17T11:04:18Z

needs rebase

laanwj · 2016-10-11T09:48:19Z

src/bitcoin-cli.cpp

@@ -294,6 +294,8 @@ int CommandLineRPC(int argc, char *argv[])
    catch (const boost::thread_interrupted&) {


Does this still need to catch boost::thread_interrupted? (another one below)

No, see "future changes" in the OP. There are a bunch of these to get rid of. I guess it doesn't make sense to do that later, I'll add it here.

Sorry, yes you mention it there. It just looked strange to me to add something instead of replace.

fanquake · 2016-11-06T12:30:06Z

Concept ACK. Needs a rebase. Tagging for 0.14.0

fanquake · 2016-11-12T12:53:06Z

There also some comments/examples about using boost thread in scheduler that could be nuked.

theuni · 2016-12-06T03:01:52Z

Closing for now, need to get a specific chunk of this in first.

fanquake added the Refactoring label Aug 31, 2016

theuni force-pushed the no-interrupt-threads branch from e25e118 to 9fb3b1c Compare September 1, 2016 00:07

theuni force-pushed the no-interrupt-threads branch 2 times, most recently from 0115d72 to c15e11b Compare September 2, 2016 05:01

theuni force-pushed the no-interrupt-threads branch 2 times, most recently from 789eaff to 8a3bdd5 Compare September 2, 2016 06:09

theuni added 11 commits September 2, 2016 08:38

threads: turn upnp into a std::thread

d083ab9

threads: make scheduler interruptible

8447a96

threads: make dnsseed interruptible

ed68c68

threads: eliminate the need for this_thread::interruption_point

61f7171

Also note that boost::thread catches boost::thread_interrupted itself, but std::thread obviously won't catch thread_interrupted. So don't rethrow after catching.

threads: use std::thread for net threads

54cb55b

Also, drop threadGroup for them and treat them individually

threads: add interruptible sleep for net threads

d20fe53

threads: turn MilliSleep into a wrapper around std::this_thread::slee…

3f31fe5

…p_for

threads: use std::thread for torcontrol

d798f58

threads: make script check threads interruptible

eb126ca

threads: use std::thread for threadflushing

8e1347a

threads: use a std::thread for importing

1fa9450

JeremyRubin reviewed Sep 5, 2016
View reviewed changes

laanwj mentioned this pull request Sep 14, 2016

Replace BOOST_FOREACH with C++11's for loop #8718

Closed

fanquake mentioned this pull request Oct 5, 2016

Bump minimum required Boost version #8875

Closed

laanwj reviewed Oct 11, 2016

View reviewed changes

fanquake added this to the 0.14.0 milestone Nov 6, 2016

fanquake mentioned this pull request Nov 12, 2016

Outstanding cpp11 refactors #8350

Closed

theuni closed this Dec 6, 2016

theuni mentioned this pull request Dec 6, 2016

net: drop boost::thread_group #9289

Merged

OlegGirko mentioned this pull request Aug 9, 2017

Backport Bitcoin PR#9289: net: drop boost::thread_group dashpay/dash#1568

Merged

meshcollider mentioned this pull request Oct 16, 2018

refactor: Drop boost::thread and boost::chrono #14489

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -294,6 +294,8 @@ int CommandLineRPC(int argc, char *argv[])
		catch (const boost::thread_interrupted&) {

Nuke boost::thread and boost::thread_group #8631

Nuke boost::thread and boost::thread_group #8631

Uh oh!

Conversation

theuni commented Aug 31, 2016

Background

Changes

Future changes

Uh oh!

JeremyRubin commented Aug 31, 2016

Uh oh!

theuni commented Aug 31, 2016

Uh oh!

JeremyRubin commented Aug 31, 2016

Uh oh!

sipa commented Aug 31, 2016

Uh oh!

paveljanik commented Aug 31, 2016

Uh oh!

paveljanik commented Aug 31, 2016

Uh oh!

sipa commented Aug 31, 2016

Uh oh!

theuni commented Sep 1, 2016

Uh oh!

laanwj commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 1, 2016

Uh oh!

theuni commented Sep 1, 2016

Uh oh!

theuni commented Sep 2, 2016

Uh oh!

btcdrak commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin Sep 5, 2016

Choose a reason for hiding this comment

Uh oh!

theuni Sep 6, 2016

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Sep 5, 2016

Uh oh!

theuni commented Sep 6, 2016

Uh oh!

JeremyRubin commented Sep 6, 2016

Uh oh!

theuni commented Sep 6, 2016

Uh oh!

JeremyRubin commented Sep 6, 2016

Uh oh!

btcdrak commented Sep 17, 2016

Uh oh!

laanwj Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

theuni Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 6, 2016

Uh oh!

fanquake commented Nov 12, 2016

Uh oh!

theuni commented Dec 6, 2016

Uh oh!

Uh oh!

laanwj commented Sep 1, 2016 •

edited

Loading

btcdrak commented Sep 2, 2016 •

edited

Loading