[Base] Fix WaitMultiple on POSIX

- Never use `cond_.notify_one()` because it may wake a thread that is
  unrelated to the signalled wait handle, resulting in a lost wake and
  possible deadlock. Wait conditions are to be checked by the threads
  themselves.
- Refactor and simplify `WaitMultiple`
This commit is contained in:
Joel Linn
2022-02-21 23:09:20 +01:00
committed by Rick Gibbed
parent ca6296089e
commit bb42829308

View File

@@ -2,7 +2,7 @@
****************************************************************************** ******************************************************************************
* Xenia : Xbox 360 Emulator Research Project * * Xenia : Xbox 360 Emulator Research Project *
****************************************************************************** ******************************************************************************
* Copyright 2020 Ben Vanik. All rights reserved. * * Copyright 2022 Ben Vanik. All rights reserved. *
* Released under the BSD license - see LICENSE in the root for more details. * * Released under the BSD license - see LICENSE in the root for more details. *
****************************************************************************** ******************************************************************************
*/ */
@@ -253,37 +253,37 @@ class PosixConditionBase {
static std::pair<WaitResult, size_t> WaitMultiple( static std::pair<WaitResult, size_t> WaitMultiple(
std::vector<PosixConditionBase*>&& handles, bool wait_all, std::vector<PosixConditionBase*>&& handles, bool wait_all,
std::chrono::milliseconds timeout) { std::chrono::milliseconds timeout) {
using iter_t = std::vector<PosixConditionBase*>::const_iterator; assert_true(handles.size() > 0);
bool executed;
auto predicate = [](auto h) { return h->signaled(); };
// Construct a condition for all or any depending on wait_all // Construct a condition for all or any depending on wait_all
auto operation = wait_all ? std::all_of<iter_t, decltype(predicate)> std::function<bool()> predicate;
: std::any_of<iter_t, decltype(predicate)>; {
auto aggregate = [&handles, operation, predicate] { using iter_t = std::vector<PosixConditionBase*>::const_iterator;
return operation(handles.cbegin(), handles.cend(), predicate); const auto predicate_inner = [](auto h) { return h->signaled(); };
}; const auto operation =
wait_all ? std::all_of<iter_t, decltype(predicate_inner)>
: std::any_of<iter_t, decltype(predicate_inner)>;
predicate = [&handles, operation, predicate_inner] {
return operation(handles.cbegin(), handles.cend(), predicate_inner);
};
}
// TODO(bwrsandman, Triang3l) This is controversial, see issue #1677 // TODO(bwrsandman, Triang3l) This is controversial, see issue #1677
// This will probably cause a deadlock on the next thread doing any waiting // This will probably cause a deadlock on the next thread doing any waiting
// if the thread is suspended between locking and waiting // if the thread is suspended between locking and waiting
std::unique_lock<std::mutex> lock(PosixConditionBase::mutex_); std::unique_lock<std::mutex> lock(PosixConditionBase::mutex_);
// Check if the aggregate lambda (all or any) is already satisfied bool wait_success = true;
if (aggregate()) { // If the timeout is infinite, wait without timeout.
executed = true; // The predicate will be checked before beginning the wait
if (timeout == std::chrono::milliseconds::max()) {
PosixConditionBase::cond_.wait(lock, predicate);
} else { } else {
// If the aggregate is not yet satisfied and the timeout is infinite, // Wait with timeout.
// wait without timeout. wait_success =
if (timeout == std::chrono::milliseconds::max()) { PosixConditionBase::cond_.wait_for(lock, timeout, predicate);
PosixConditionBase::cond_.wait(lock, aggregate);
executed = true;
} else {
// Wait with timeout.
executed = PosixConditionBase::cond_.wait_for(lock, timeout, aggregate);
}
} }
if (executed) { if (wait_success) {
auto first_signaled = std::numeric_limits<size_t>::max(); auto first_signaled = std::numeric_limits<size_t>::max();
for (auto i = 0u; i < handles.size(); ++i) { for (auto i = 0u; i < handles.size(); ++i) {
if (handles[i]->signaled()) { if (handles[i]->signaled()) {
@@ -294,6 +294,7 @@ class PosixConditionBase {
if (!wait_all) break; if (!wait_all) break;
} }
} }
assert_true(std::numeric_limits<size_t>::max() != first_signaled);
return std::make_pair(WaitResult::kSuccess, first_signaled); return std::make_pair(WaitResult::kSuccess, first_signaled);
} else { } else {
return std::make_pair<WaitResult, size_t>(WaitResult::kTimeout, 0); return std::make_pair<WaitResult, size_t>(WaitResult::kTimeout, 0);
@@ -329,13 +330,7 @@ class PosixCondition<Event> : public PosixConditionBase {
bool Signal() override { bool Signal() override {
auto lock = std::unique_lock<std::mutex>(mutex_); auto lock = std::unique_lock<std::mutex>(mutex_);
signal_ = true; signal_ = true;
if (manual_reset_) { cond_.notify_all();
cond_.notify_all();
} else {
// FIXME(bwrsandman): Potential cause for deadlock
// See issue #1678 for possible fix and discussion
cond_.notify_one();
}
return true; return true;
} }
@@ -402,7 +397,7 @@ class PosixCondition<Mutant> : public PosixConditionBase {
--count_; --count_;
// Free to be acquired by another thread // Free to be acquired by another thread
if (count_ == 0) { if (count_ == 0) {
cond_.notify_one(); cond_.notify_all();
} }
return true; return true;
} }
@@ -471,11 +466,7 @@ class PosixCondition<Timer> : public PosixConditionBase {
// Store callback // Store callback
if (callback_) callback = callback_; if (callback_) callback = callback_;
signal_ = true; signal_ = true;
if (manual_reset_) { cond_.notify_all();
cond_.notify_all();
} else {
cond_.notify_one();
}
} }
// Call callback // Call callback
if (callback) callback(); if (callback) callback();