RFC: Interruptible threads #8023

theuni · 2016-05-08T09:00:03Z

The intention here is to create a drop-in replacement for:
boost::thread
boost::thread_group
boost::condition_variable
boost::this_thread

These are just interruptible wrappers around the c++11 std:: versions. There are no dependencies, and afaik it's completely portable.

This allows us to move away from boost threads (which block the movement of other things like mutexes, function, bind, chrono, etc), without having to worry about refactoring to cope with our dependence on interruption points, interruptible condition variables, and interruptible sleeps.

For the places that don't require interruption, they can just move directly to std::.

The code needs some refinements, comments, and more tests, but I'd like to get a few concept ack's before continuing. Obviously the next step is to start plugging it all in.

Also, to avoid conflicting with segwit changes and backporting, it may be easier to replace things in some places and not others.

The condition_variable is actually an implementation of condition_variable_any, which means that it will accept any Lockable. So we can mix and match boost::mutex/boost::unique_lock/std::mutex/std::unique_lock without any trouble, if that eases the migration.

These should be drop-in replacements for: boost::thread boost::thread_group boost::condition_variable boost::this_thread::sleep_for

kazcw · 2016-05-08T15:08:14Z

thread is the only owner of the m_data shared pointer, but this_thread is given a raw pointer to the same object. If the thread is destroyed while the thread is still running, it will destroy the thread_data object the thread's still using via this_thread's pointer.

kazcw · 2016-05-08T15:44:39Z

Boost's thread_group.remove_thread doesn't destroy the thread object.

kazcw · 2016-05-08T16:45:47Z

src/interruptible/condition_variable.h

+        while (!pred()) {
+            bool ret = m_condvar.wait_until(lock, abs_time) == std::cv_status::timeout;
+            this_thread::interruption_point();
+            if (!ret)


The loop spins after timeout. Should this test be if (ret)?

as kazcw points out, the raw thread may be running free after the interruptible has been destroyed, so its data needs to remain valid. TODO: This probably isn't enough to cope with detaching, need to poke at it and add some tests.

Clarify some wait_until, and make doubly-sure that exceptions have been thrown.

Thanks to kazcw for pointing out that boost's thread_group only deletes threads in its own dtor. Also updated create_thread to return a pointer to the thread to match the boost functionality.

theuni · 2016-05-08T17:17:52Z

@kazcw Thanks for the great review! It's much appreciated. This surely needs lots more tests, and there are still some rough edges.

I'll be away for a few days, if others agree this is a good plan, I'll work on cleanup and test coverage when I get back.

kazcw · 2016-05-08T18:33:54Z

src/interruptible/thread_data.h

+
+    void set_interrupted()
+    {
+        m_interrupted.store(true, std::memory_order_relaxed);


This needs to be at least memory_order_acquire so it happens before the notify. Locking the mutex is only an acquire operation and won't act as a suitable fence.

Yep, thanks. This and the load should've both been marked for TODO, as I don't understand their interaction with the notify well enough. The orderings seem like pure voodoo :)

Yeah, it's wild stuff. I don't really understand it either.

The synchronization effects of notify on non-condition_variable values aren't specified, so I think we have to be conservative. One approach would be a release fence after the store and an acquire load operation. There may be marginally better-performing options on the store side, but this has the benefit that acquire/release pairs are relatively comprehensible. The load could technically be relaxed and the compiler still "should" make sure thread that's doing the loads sees the update "in a reasonable amount of time", but the compiler can hoist relaxed loads out of loops if it sees fit and who knows what's a reasonable amount of time to a compiler. Acquire and release are both cheap on x86 and anyway, interrupt points are going to be places that we expect time consuming operations to be happening, not in tight loops.

I've been thinking about this... I think it was actually fine as is. The store and the notify don't need to be ordered relative to each other because the release of the lock at the exit of the scope is synchronized-with the acquire of the lock that wait must perform before returning.

kazcw · 2016-05-08T18:44:50Z

Concept ACK on moving away from boost for threading.

Since most threads are in the same thread_group, we'd have to switch over almost everything at once, right? Shouldn't be too hard with a drop-in replacement though...

kazcw · 2016-05-08T19:24:55Z

The relationship between int_lock and thread_data does seem a bit funky, especially around the m_wake_mutex lock. What about switching the ownership: unique_lock in thread_data, reference to it in the int_lock? int_lock could become a generic utility class that takes two lock references and dispatch to both of them in tandem. condition_variable would need to set_cond after taking the double-lock, but I think it's a clearer division of the concerns.

theuni · 2016-05-08T20:28:28Z

@kazcw The problem with that is that you still have to access the private condvar pointer after grabbing the double-lock, or use a getter that provides unguarded access, so the lock isn't actually providing any protection other than the understanding that the caller will use it to change the pointer (If I'm understanding you correctly). I tried that approach, and it led to the creation of an RAII wrapper holding a unique_lock and condition_variable_any which null'd the pointer at destruction. But The concept of grabbing a lock just to pass into another lock to be combined with a 3rd user lock was just too wonky.

But by all means, I'd be happy to see something more intuitive than what's here now :)

kazcw · 2016-05-09T01:29:00Z

I see. Yeah, I don't see any way for thread_data to expose a data-race-safe API that doesn't require some funky business. The current approach would work fine.

TheBlueMatt · 2016-05-11T23:08:07Z

I appreciate all the work thats gone into this, and I think it should be merged eventually, but as our agreed-upon timeline has been that only new code should use C++11 features until after 0.13, I have to say NACK.

laanwj · 2016-05-20T14:13:58Z

Re: C++11 changes - changing utility classes to use C++11 is fine as long as the code using it is not or hardly impacted (e.g. the change to make tinyformat use C++11 variadic templates). But here we have to change all use/call sites as well (unless we do a terrible hack to disguise as boost) so yes, need to postpone this a bit.

But Concept ACK. Makes sense for 0.14 along with more compatibility changes to get rid of boost. Added the "future" milestone.

7e87033 httpserver: replace boost threads with std (Cory Fields) d3773ca httpserver: explicitly detach worker threads (Cory Fields) 755aa05 httpserver: use a future rather than relying on boost's try_join_for (Cory Fields)

HTTP Server cherries from Core: bitcoin/bitcoin#6719 - Make HTTP server shutdown more graceful bitcoin/bitcoin#6859 - http: Restrict maximum size of http + headers bitcoin/bitcoin#6990 - http: speed up shutdown bitcoin/bitcoin#7966 - http: Do a pending c++11 simplification handling work items bitcoin/bitcoin#8421 - httpserver: drop boost (#8023 dependency) bitcoin/bitcoin#11006 - Improve shutdown process

7e87033 httpserver: replace boost threads with std (Cory Fields) d3773ca httpserver: explicitly detach worker threads (Cory Fields) 755aa05 httpserver: use a future rather than relying on boost's try_join_for (Cory Fields)

theuni added 2 commits May 8, 2016 04:42

threads: add interruptible threads

4831035

These should be drop-in replacements for: boost::thread boost::thread_group boost::condition_variable boost::this_thread::sleep_for

threads: add some tests for interruptible threads

33dd6c8

kazcw reviewed May 8, 2016
View reviewed changes

theuni added 3 commits May 8, 2016 13:04

squashme: fixup thread_data pointer

bb31ffb

as kazcw points out, the raw thread may be running free after the interruptible has been destroyed, so its data needs to remain valid. TODO: This probably isn't enough to cope with detaching, need to poke at it and add some tests.

squashme: cleanup tests

383e087

Clarify some wait_until, and make doubly-sure that exceptions have been thrown.

squashme: fixup thread_group

584cd8c

Thanks to kazcw for pointing out that boost's thread_group only deletes threads in its own dtor. Also updated create_thread to return a pointer to the thread to match the boost functionality.

squashme: fixup condition_variable::wait_until

6413320

kazcw reviewed May 8, 2016
View reviewed changes

jonasschnelli added the Refactoring label May 8, 2016

laanwj added this to the Future milestone May 20, 2016

laanwj modified the milestones: 0.14, Future Jun 22, 2016

theuni mentioned this pull request Jul 28, 2016

httpserver: drop boost (#8023 dependency) #8421

Merged

theuni closed this Aug 31, 2016

theuni mentioned this pull request Aug 31, 2016

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

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RFC: Interruptible threads #8023

RFC: Interruptible threads #8023

Uh oh!

theuni commented May 8, 2016

Uh oh!

kazcw commented May 8, 2016

Uh oh!

kazcw commented May 8, 2016

Uh oh!

kazcw May 8, 2016

Uh oh!

theuni May 8, 2016

Uh oh!

theuni commented May 8, 2016

Uh oh!

kazcw May 8, 2016

Uh oh!

theuni May 8, 2016

Uh oh!

kazcw May 9, 2016

Uh oh!

kazcw May 11, 2016

Uh oh!

kazcw commented May 8, 2016

Uh oh!

kazcw commented May 8, 2016

Uh oh!

theuni commented May 8, 2016

Uh oh!

kazcw commented May 9, 2016

Uh oh!

TheBlueMatt commented May 11, 2016

Uh oh!

laanwj commented May 20, 2016

Uh oh!

Uh oh!

RFC: Interruptible threads #8023

RFC: Interruptible threads #8023

Uh oh!

Conversation

theuni commented May 8, 2016

Uh oh!

kazcw commented May 8, 2016

Uh oh!

kazcw commented May 8, 2016

Uh oh!

kazcw May 8, 2016

Choose a reason for hiding this comment

Uh oh!

theuni May 8, 2016

Choose a reason for hiding this comment

Uh oh!

theuni commented May 8, 2016

Uh oh!

kazcw May 8, 2016

Choose a reason for hiding this comment

Uh oh!

theuni May 8, 2016

Choose a reason for hiding this comment

Uh oh!

kazcw May 9, 2016

Choose a reason for hiding this comment

Uh oh!

kazcw May 11, 2016

Choose a reason for hiding this comment

Uh oh!

kazcw commented May 8, 2016

Uh oh!

kazcw commented May 8, 2016

Uh oh!

theuni commented May 8, 2016

Uh oh!

kazcw commented May 9, 2016

Uh oh!

TheBlueMatt commented May 11, 2016

Uh oh!

laanwj commented May 20, 2016

Uh oh!

Uh oh!