Fixed UpgradedHolder-related race condition in ActiveHostsMan #622
Conversation
dutor
requested review from
monadbobo,
CPWstatic,
critical27,
wadeliuyi,
dangleptr and
laura-ding
|
Unit testing passed. |
src/meta/ActiveHostsMan.cpp
Outdated
| auto it = hostsMap_.find(hostAddr); | ||
| if (it == hostsMap_.end()) { | ||
| folly::RWSpinLock::UpgradedHolder uh(&lock_); | ||
| WriteHolder wh(UpgradedHolder(&lock_)); |
dutor
dismissed stale reviews from boshengchen, wadeliuyi, and monadbobo
via
01b62b9
|
Here is a snippet to demo this issue. folly::RWSpinLock lock;
auto reader = std::thread([&] () {
folly::RWSpinLock::ReadHolder rh(lock);
fprintf(stderr, "read locked\n");
sleep(5);
fprintf(stderr, "read unlocked\n");
});
auto writer = std::thread([&]() {
sleep(1);
folly::RWSpinLock::ReadHolder rh(lock);
folly::RWSpinLock::UpgradedHolder uh(lock);
fprintf(stderr, "upgraded\n");
//rh.reset();
//folly::RWSpinLock::WriteHolder wh(std::move(uh));
//fprintf(stderr, "write locked\n");
sleep(10);
});
reader.join();
writer.join(); |
|
@monadbobo @wadeliuyi @boshengchen Please note that the first commit of this PR is buggy. It has a deadlock problem, because the writer-to-be thread, aka. the current thread, is a reader itself. So it must release the read lock before acquire the write lock. |
|
Unit testing failed. |
|
Unit testing passed. |
src/meta/ActiveHostsMan.cpp
Outdated
| // Step to Upgraded phase to stop new readers | ||
| UpgradedHolder uh(&lock_); | ||
| // Unlock the read lock | ||
| rh.reset(); |
There was a problem hiding this comment.
Is hostsMap_ the single-writer-many-reader?
There was a problem hiding this comment.
It is multi-writer, update by heartbeat.
There was a problem hiding this comment.
No, it's a multi-writer multi-reader case
|
For simplicity, we decide to use If anyone has questions about the |
|
Unit testing passed. |
|
Thanks for taking care of it. @dutor For more information about UpgradeLock concept, please refer https://www.boost.org/doc/libs/1_58_0/doc/html/thread/synchronization.html#thread.synchronization.mutex_concepts.upgrade_lockable PS. The implementation in folly is different with boost. |
…-inc#622) * Fixed UpgradedHolder-releated race condition in ActiveHostsMan * unlock own readlock before acquire write lock * fix dependencies * For simplicity, use WriteHolder instead
…-inc#622) * Fixed UpgradedHolder-releated race condition in ActiveHostsMan * unlock own readlock before acquire write lock * fix dependencies * For simplicity, use WriteHolder instead
Co-authored-by: Dashuang Li <ldshuang@gmail.com>
UpgradedHoldermakes aRWSpinLockorReadHolderprogress to theUPGRADEDphase. AndUPGRADEDis an intermediate phase to transit(upgrade) fromREADtoWRITE.