refactor: Replace boost::mutex,condition_var,chrono with std equivalents in scheduler #18234

ajtowns · 2020-03-01T05:09:41Z

Replacing boost functionality with C++11 stuff.

Motivated by #18227, but should stand alone. Changing from boost::condition_var to std::condition_var means threadGroup.interrupt_all isn't enough to interrupt serviceQueue anymore, so that means calling stop() before join_all() is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from g_lockstack which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour.

Fixes #16027, Fixes #14200, Fixes #18227

fanquake · 2020-03-01T05:14:31Z

cc @theuni. I know you have been working on similar refactors recently.

DrahtBot · 2020-03-01T10:03:49Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

scheduler: Workaround negative nsecs bug in boost's wait_until #18271 (scheduler: Workaround negative nsecs bug in boost's wait_until by luke-jr)
build: Remove Boost Chrono #18264 ([WIP] build: Remove Boost Chrono by fanquake)
util: Replace boost sleep with std sleep #16117 (util: Replace boost sleep with std sleep by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

elichai · 2020-03-01T13:29:17Z

src/sync.h

Pending comments on my post here: #18088 (comment)
Maybe replace it with PASTE2(revlock,PASTE2(__FILE__,__LINE__))

We already use __COUNTER__ for LOCK() in sync.h, so reusing it here seems fine. Using __FILE__ seems unlikely to generate a usable symbol name too :)

src/sync.cpp

practicalswift · 2020-03-01T20:14:09Z

Concept ACK

Thanks for working on de-boosting! :)

jonatack

ACK 42a5326 I reviewed the changes and surrounding code in detail, built/ran tests for each commit, ran bitcoind for 2 days (debian), and shut down/restarted bitcoind several times. This looks good.

jonatack · 2020-03-02T14:22:43Z

src/test/txindex_tests.cpp

In 69460c9 could these be also removed in src/test/transaction_tests.cpp L::457-458 if they are handled with TestingSetup::~TestingSetup(); the test compiles and runs without them.

--- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -453,9 +453,6 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) bool controlCheck = control.Wait(); assert(controlCheck); - - threadGroup.interrupt_all(); - threadGroup.join_all(); }

Not sure why this should be safe to remove. The scheduler needs to be stopped before this test ends, so the interrupt_all should be replaced by a scheduler.stop(). I think that otherwise you reintroduce bug #15410

Thanks. I was just now looking at src/test/checkqueue_tests.cpp which has similar code.

@jonatack In transaction_tests, that's a locally declared threadGroup, and it's a BasicTestingSetup too, so the TestingSetup destructor wouldn't execute, and even if it did, wouldn't clean up that threadGroup.

@MarcoFalke Ah, you're quite right. Have undeleted the code and added a comment.

@ajtowns right, makes sense. Thanks for the explanation.

in commit 306f71b:

After adding the scheduler.stop(), they are again redundant and confusing. I'd suggest to remove the threadGroup calls

Calling stop() doesn't actually cause the thread to stop, you need to wait for it to complete. Could do that by looping and waiting for AreThreadsServicingQueue() to be false, but that still leaves the possibility you continue on and destruct the object in between --nThreadsServicingQueue; and newTaskScheduled.notify_one(); at the end of serviceQueue. Could drop the interrupt_all but if some other thread in the group needed an interrupt before being joined, that'd cause a hang too... So until we change something else, this doesn't seem redundant to me?

Ups, correct.

theuni · 2020-03-02T16:30:46Z

~~This doesn't seem to take into account that boost::condition_variable is interruptible, and can't simply be swapped out. Have I missed some prior work that makes this ok?~~

Ok, I see that in the description. Re-reviewing.

theuni · 2020-03-02T17:03:39Z

I would've expected this to require more work, but it looks like the scheduler was already surprisingly interruptible. Concept ACK.

@TheBlueMatt might be aware of more scheduling minefields.

src/scheduler.cpp

theuni · 2020-03-02T18:52:02Z

src/init.cpp

Are these guaranteed to fully execute in-order? Can an interruption-point hit while the scheduler is still tearing down?

Not sure what you're asking; node.scheduler->stop() just sets a boolean to say "exit the loop" and notifies everything waiting on the condition var to prevent it staying asleep, all the tear down happens in the thread. The interruption could certainly happen while it's tearing down, stop(); interrupt_all() happen in one thread, and only afterwards does the other thread even wake up. I don't think boost interrupt will have an effect anymore though, because serviceQueue doesn't use any boost primitives and boost interrupt only sets a flag which is checked by other boost things.

maflcko · 2020-03-05T21:03:26Z

Should add

Fixes #16027, Fixes #14200, Fixes #18227

to OP?

fanquake · 2020-03-06T07:42:48Z

@ajtowns Can you rebase on master now that #16117 is merged. Also, feel free to include a commit here to kill the last DHAVE_WORKING_BOOST_SLEEP_FOR.

Calling interrupt_all() will immediately stop the scheduler, so it's safe to invoke stop() beforehand, and this removes the reliance on boost to interrupt serviceQueue().

Changes from boost::chrono to std::chrono, boost::condition_var to std::condition_var, boost::mutex to sync.h Mutex, and reverselock.h to sync.h REVERSE_LOCK. Also adds threadsafety annotations to CScheduler members.

laanwj · 2020-03-06T19:08:06Z

Great work! I'm really glad this is happening, thanks for working on this.
ACK 70a6b52

maflcko

ACK 70a6b52, just style-nits 🌩

Show signature and timestamp

Signature:

Timestamp of file with hash 8b7a37edb1d87523e3df70995e0c51ee4dd4dafc50f78c0b12195a66e49c4ce2 -

maflcko · 2020-03-17T15:00:30Z

src/test/txindex_tests.cpp

in commit 306f71b:

After adding the scheduler.stop(), they are again redundant and confusing. I'd suggest to remove the threadGroup calls

maflcko · 2020-03-17T15:05:00Z

src/sync.cpp

    push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry, util::ThreadGetInternalName()));
 }

+void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line)


in commit b9c4260:

Why is this not called GetLastLockName, which better describes what this function does, imo.

maflcko · 2020-03-17T15:09:42Z

src/sync.h

+
+        UniqueLock& lock;
+        UniqueLock templock;
+        std::string lockname;


in commit b9c4260:

Should be m_lockname. Also for other members?

maflcko · 2020-03-17T16:40:04Z

src/sync.h

+        reverse_lock& operator=(reverse_lock const&);
+
+        UniqueLock& lock;
+        UniqueLock templock;


in commit b9c4260:

I think it would help readers of the code to briefly explain why this is needed. Also, m_templock? ;)

maflcko · 2020-03-17T16:45:23Z

src/scheduler.h

 //
 // NOTE:
-// boost::thread / boost::chrono should be ported to std::thread / std::chrono
+// boost::thread should be ported to std::thread


in commit d0ebd93:

Actually boost::thread is no longer needed an can be replaced with std::thread in this file and the scheduler tests. I know that the thread group is still boost, but the scheduler doesn't care about that.

Summary: Calling interrupt_all() will immediately stop the scheduler, so it's safe to invoke stop() beforehand, and this removes the reliance on boost to interrupt serviceQueue(). This is a partial abckport of Core [[bitcoin/bitcoin#18234 | PR18234]] : bitcoin/bitcoin@306f71b Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6531

Summary: This is a partial backport of Core [[bitcoin/bitcoin#18234 | PR18234]] : bitcoin/bitcoin@b9c4260 Depends on D6531 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6532

Summary: Changes from boost::chrono to std::chrono, boost::condition_var to std::condition_var, boost::mutex to sync.h Mutex, and reverselock.h to sync.h REVERSE_LOCK. Also adds threadsafety annotations to CScheduler members. This is a partial backport of Core [[bitcoin/bitcoin#18234 | PR18234]] : bitcoin/bitcoin@d0ebd93 Depends on D6532 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6533

Summary: This is a partial backport of Core [[bitcoin/bitcoin#18234 | PR18234]] : bitcoin/bitcoin@cea19f6 Depends on D6533 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6535

…t's wait_until ed0223e scheduler: Workaround negative nsecs bug in boost's wait_until (Luke Dashjr) Pull request description: Some boost versions have a bug that can cause a time prior to system boot (or wake from sleep) to throw an exception instead of return timeout See boostorg/thread#308 NOTE: This was addressed in master with a refactor (#18234), so this isn't a strict backport and needs full review. Fixes #18227 Cleanly merges to 0.14+ ACKs for top commit: laanwj: ACK ed0223e gruve-p: ACK ed0223e Tree-SHA512: 57edd0a22d7cf8f04b427e23d1ba10746a492638021d4438781b9d313dd0459418f64f0489be72d8e2286bbc8e8762d77e673868c25eb3bf4f0423a8fe8cdffa

Summary: > After [[bitcoin/bitcoin#18234 | core#18234]] the scheduler itself no longer cares if the > serviceQueue is run in a std::thread or boost::thread. Change the > documentation to std::thread because we switched to C++11. This is a backport of [[bitcoin/bitcoin#19090 | core#19090]] [3/6] bitcoin/bitcoin@fab2950 Test Plan: Proofreading Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9957

… std equivalents in scheduler b2ac70f [Cleanup] Drop unused reverselock.h (random-zebra) 55ac6de scheduler: switch from boost to std (Anthony Towns) 15f292b sync.h: add REVERSE_LOCK (Anthony Towns) a8705a1 scheduler: don't rely on boost interrupt on shutdown (random-zebra) 004c064 util: Add UnintrruptibleSleep (MarcoFalke) Pull request description: Backports bitcoin#18234 [Anthony Towns] > Motivated by 18227, but should stand alone. Changing from boost::condition_var to std::condition_var means threadGroup.interrupt_all isn't enough to interrupt serviceQueue anymore, so that means calling stop() before join_all() is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from g_lockstack which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour. Fixes 16027, Fixes 14200, Fixes 18227 First commit coming from bitcoin#16117 This should fix #2515 ACKs for top commit: furszy: code review ACK b2ac70f Fuzzbawls: utACK b2ac70f Tree-SHA512: 5a39f8047948d02f75bb3cce99b8a9ce8d31e30c2e12a8ebfe2a871ed8e0fbe552c6e9deaad5d3e9c0830e7286f2cb627f0605d5887c4407f91cc2f9d667e8b1

…_var,chrono with std equivalents in scheduler This backport does not include changes that depend on bitcoin pr 18037 70a6b52 lint-cppcheck: Remove -DHAVE_WORKING_BOOST_SLEEP_FOR (Anthony Towns) 294937b scheduler_tests: re-enable mockforward test (Anthony Towns) cea19f6 Drop unused reverselock.h (Anthony Towns) d0ebd93 scheduler: switch from boost to std (Anthony Towns) b9c4260 sync.h: add REVERSE_LOCK (Anthony Towns) 306f71b scheduler: don't rely on boost interrupt on shutdown (Anthony Towns) Pull request description: Replacing boost functionality with C++11 stuff. Motivated by bitcoin#18227, but should stand alone. Changing from `boost::condition_var` to `std::condition_var` means `threadGroup.interrupt_all` isn't enough to interrupt `serviceQueue` anymore, so that means calling `stop()` before `join_all()` is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from `g_lockstack` which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour. Fixes bitcoin#16027, Fixes bitcoin#14200, Fixes bitcoin#18227 ACKs for top commit: laanwj: ACK 70a6b52 Tree-SHA512: d1da13adeabcf9186d114e2dad9a4fdbe2e440f7afbccde0c13dfbaf464efcd850b69d3371c5bf8b179d7ceb9d81f4af3cc22960b90834e41eaaf6d52ef7d331 # Conflicts: # src/reverselock.h # src/rpc/misc.cpp # src/scheduler.cpp # src/scheduler.h # src/sync.cpp # src/sync.h # src/test/reverselock_tests.cpp # src/test/scheduler_tests.cpp # src/test/test_dash.cpp # test/lint/extended-lint-cppcheck.sh

fanquake added the Refactoring label Mar 1, 2020

ajtowns mentioned this pull request Mar 1, 2020

scheduler test causing pthread_cond_timedwait: Invalid argument error #18227

Closed

ajtowns force-pushed the 202002-scheduler-deboost branch 2 times, most recently from ff3821b to 42a5326 Compare March 1, 2020 06:03

DrahtBot mentioned this pull request Mar 1, 2020

util: Replace boost sleep with std sleep #16117

Merged

elichai reviewed Mar 1, 2020

View reviewed changes

jonatack reviewed Mar 2, 2020

View reviewed changes

theuni reviewed Mar 2, 2020

View reviewed changes

src/scheduler.cpp Outdated Show resolved Hide resolved

theuni reviewed Mar 2, 2020

View reviewed changes

maflcko changed the title ~~scheduler.cpp: Replace boost::mutex,condition_var,chrono with std equivalents~~ refactor: Replace boost::mutex,condition_var,chrono with std equivalents in scheduler Mar 3, 2020

maflcko mentioned this pull request Mar 3, 2020

Future PRs bitcoin-core-review-club/website#14

Closed

ajtowns force-pushed the 202002-scheduler-deboost branch from 42a5326 to 9426759 Compare March 4, 2020 04:03

fanquake mentioned this pull request Mar 5, 2020

build: Remove Boost Chrono #18264

Merged

maflcko added this to the 0.20.0 milestone Mar 5, 2020

maflcko mentioned this pull request Mar 5, 2020

scheduler: crash after releasing wallet #16307

Closed

DrahtBot mentioned this pull request Mar 6, 2020

scheduler: Workaround negative nsecs bug in boost's wait_until #18271

Closed

DrahtBot added the Needs rebase label Mar 6, 2020

ajtowns added 5 commits March 6, 2020 23:13

scheduler: don't rely on boost interrupt on shutdown

306f71b

Calling interrupt_all() will immediately stop the scheduler, so it's safe to invoke stop() beforehand, and this removes the reliance on boost to interrupt serviceQueue().

sync.h: add REVERSE_LOCK

b9c4260

scheduler: switch from boost to std

d0ebd93

Changes from boost::chrono to std::chrono, boost::condition_var to std::condition_var, boost::mutex to sync.h Mutex, and reverselock.h to sync.h REVERSE_LOCK. Also adds threadsafety annotations to CScheduler members.

Drop unused reverselock.h

cea19f6

scheduler_tests: re-enable mockforward test

294937b

maflcko removed the Needs rebase label Mar 6, 2020

maflcko added the Needs rebase label Mar 6, 2020

lint-cppcheck: Remove -DHAVE_WORKING_BOOST_SLEEP_FOR

70a6b52

ajtowns force-pushed the 202002-scheduler-deboost branch from 9426759 to 70a6b52 Compare March 6, 2020 18:21

DrahtBot removed the Needs rebase label Mar 6, 2020

laanwj merged commit 3516a31 into bitcoin:master Mar 6, 2020

luke-jr mentioned this pull request Mar 6, 2020

[0.19] scheduler: Workaround negative nsecs bug in boost's wait_until #18284

Merged

maflcko reviewed Mar 17, 2020

View reviewed changes

hebasto mentioned this pull request Mar 20, 2020

wallet: Remove calls to Chain::Lock methods #17954

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

UdjinM6 mentioned this pull request May 23, 2020

Fix ProcessNewBlock vs EnforceBestChainLock deadlocks in ActivateBestChain dashpay/dash#3492

Merged

jnewbery mentioned this pull request May 26, 2020

wallet: Remove boost from PeriodicFlush #18792

Merged

div72 mentioned this pull request Sep 15, 2020

gridcoinresearch crashes after the machine is woken up gridcoin-community/Gridcoin-Research#1887

Closed

random-zebra mentioned this pull request Aug 18, 2021

refactor: Replace boost::mutex,condition_var,chrono with std equivalents in scheduler PIVX-Project/PIVX#2517

Merged

This was referenced Sep 6, 2021

Boost usage reduction DeFiCh/ain#714

Closed

C++17 and reduce Boost usage DeFiCh/ain#701

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

refactor: Replace boost::mutex,condition_var,chrono with std equivalents in scheduler #18234

refactor: Replace boost::mutex,condition_var,chrono with std equivalents in scheduler #18234

Uh oh!

Conversation

ajtowns commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Mar 1, 2020

Uh oh!

DrahtBot commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Mar 1, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Mar 2, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 5, 2020

Uh oh!

fanquake commented Mar 6, 2020

Uh oh!

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

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ajtowns commented Mar 1, 2020 •

edited

Loading

DrahtBot commented Mar 1, 2020 •

edited

Loading

jonatack Mar 2, 2020 •

edited

Loading

theuni commented Mar 2, 2020 •

edited

Loading

laanwj commented Mar 6, 2020 •

edited

Loading