[scheduler] Switched CScheduler to C++11 threading primitives #10182

pull tjps wants to merge 1 commits into bitcoin:master from tjps:tjps_scheduler changing 5 files +68 −85

tjps commented at 7:16 AM on April 11, 2017: contributor

Switched CScheduler off of Boost threading primitives to std::thread/condition_variable/chrono.
fanquake added the label Refactoring on Apr 11, 2017
fanquake added this to the "In progress" column in a project

in src/scheduler.cpp:56 in 788480250c outdated

  53 | +                // Unlock while calling f, so it can reschedule itself or another task
  54 |                  // without deadlocking:
  55 | -                reverse_lock<boost::unique_lock<boost::mutex> > rlock(lock);
  56 | +                lock.unlock();
  57 |                  f();
  58 | +                lock.lock();

dcousens commented at 7:20 AM on April 11, 2017:

is f guaranteed not to throw? my understanding was that was a potential reason for the use of RAII here

tjps commented at 7:41 AM on April 11, 2017:

You make a good point, I totally missed the exception safety as a feature of reverse_lock. Added it back.

laanwj commented at 7:31 AM on April 11, 2017: member

I've been tempted down the reverse_lock RAII path many times before, but in my experience the potential usage points for such a class in a codebase are usually too few to justify its existence.

reverse_lock was introduced to fix an issue with the lock not getting released in case of exceptions, and you may have reintroduced that issue by effectively reverting the code to previous state. I'd prefer to just keep it. It works, there are tests.
tjps commented at 7:32 AM on April 11, 2017: contributor

I realize that now based on @dcousens comment. I'll add it back in.
[scheduler] Switched CScheduler to C++11 threading primitives 8d09e96151
theuni commented at 2:22 PM on April 11, 2017: member

@tjps Nice work.

Unfortunately though, this is a bit more complicated than just switching out the primitives.

boost::thread is interruptible, meaning that the scheduler thread as well as the workers will terminate soonafter a call to threadGroup.interrupt_all(). When switching to std::thread and std::condition_variable, we have to handle that manually instead. That's a good thing though, less magic going on behind the scenes!

So init.cpp will need to learn how to stop the workers and the handler gracefully before making the switch. The order is important as well: worker threads may have callbacks into objects that have been destructed at exit, so they should be interrupted early in the shutdown process.
tjps commented at 7:02 PM on May 1, 2017: contributor

Sorry to let this slide - all valid points raised. Scuttling this PR to cleanup the backlog.
tjps closed this on May 1, 2017
tjps deleted the branch on May 1, 2017
fanquake moved this from the "In progress" to the "Later" column in a project

MarcoFalke moved this from the "Later" to the "Done" column in a project

MarcoFalke locked this on Sep 8, 2021

Contributors

Labels