RFC: Interruptible threads

theuni commented at 9:00 am on May 8, 2016: member

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.

threads: add interruptible threads

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

483103590f

threads: add some tests for interruptible threads 33dd6c85b9

kazcw commented at 3:08 pm on May 8, 2016: contributor

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 commented at 3:44 pm on May 8, 2016: contributor

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

squashme: fixup thread_data pointer

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.

bb31ffbda5

squashme: cleanup tests

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

383e087c48

squashme: fixup thread_group

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.

584cd8ce97

in src/interruptible/condition_variable.h: in 33dd6c85b9 outdated

72+        lock_type lock(m_condvar, user_lock);
73+        this_thread::interruption_point();
74+        while (!pred()) {
75+            bool ret = m_condvar.wait_until(lock, abs_time) == std::cv_status::timeout;
76+            this_thread::interruption_point();
77+            if (!ret)

kazcw commented at 4:45 pm on May 8, 2016:

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

theuni commented at 5:15 pm on May 8, 2016:

yep!

theuni commented at 5:17 pm on May 8, 2016: member

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

squashme: fixup condition_variable::wait_until 64133202a7

in src/interruptible/thread_data.h: in 64133202a7

30+        m_thread = std::thread(std::forward<T>(func), std::forward<Args>(args)...);
31+    }
32+
33+    void set_interrupted()
34+    {
35+        m_interrupted.store(true, std::memory_order_relaxed);

kazcw commented at 6:33 pm on May 8, 2016:

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.

theuni commented at 8:18 pm on May 8, 2016:

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 :)

kazcw commented at 1:21 am on May 9, 2016:

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.

kazcw commented at 10:34 pm on May 11, 2016:

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 commented at 6:44 pm on May 8, 2016: contributor

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 commented at 7:24 pm on May 8, 2016: contributor

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.

jonasschnelli added the label Refactoring on May 8, 2016

theuni commented at 8:28 pm on May 8, 2016: member

@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 commented at 1:29 am on May 9, 2016: contributor

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 commented at 11:08 pm on May 11, 2016: member

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 added this to the milestone Future on May 20, 2016

laanwj commented at 2:13 pm on May 20, 2016: member

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.

laanwj added this to the milestone 0.14 on Jun 22, 2016

laanwj removed this from the milestone Future on Jun 22, 2016

laanwj referenced this in commit b77bb95b3c on Jul 29, 2016

theuni closed this on Aug 31, 2016

codablock referenced this in commit 784153102c on Sep 19, 2017

codablock referenced this in commit 9df93b6f86 on Dec 29, 2017

codablock referenced this in commit c03c42116b on Jan 8, 2018

sickpig referenced this in commit 46b5b86be5 on Mar 9, 2018

sickpig referenced this in commit dadc2880a1 on Mar 9, 2018

sickpig referenced this in commit 6d4a55878a on Mar 12, 2018

sickpig referenced this in commit 6db640a393 on Mar 12, 2018

sickpig referenced this in commit 76cf243311 on Mar 12, 2018

sickpig referenced this in commit 4428056252 on Mar 12, 2018

andvgal referenced this in commit b29ac790c8 on Jan 6, 2019

DrahtBot locked this on Sep 8, 2021

RFC: Interruptible threads #8023