From 04ebc11a2d8a2e59abda5061e35e504fc30504d2 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 24 Nov 2021 12:56:48 -0500
Subject: [PATCH] SL-16094: Fix WorkQueue test for correct behavior of
runFor().
Turns out that one of our WorkQueue integration tests was relying on the
incorrect runFor() behavior that we just fixed, so the test broke. Now that
runFor() doesn't wait around for work to be posted, use an explicit wait loop
instead.
To support this, add LLCond::get(functor), where functor must accept a const
reference to the stored data. This new get() returns whatever the functor
returns, allowing a caller to peek at the stored data.
Also use universal references for all remaining LLCond functor arguments.
---
indra/llcommon/llcond.h | 52 +++++++++++++++++--------
indra/llcommon/tests/workqueue_test.cpp | 12 ++++--
2 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/indra/llcommon/llcond.h b/indra/llcommon/llcond.h
index c08acb66a1b..da6e6affe1b 100644
--- a/indra/llcommon/llcond.h
+++ b/indra/llcommon/llcond.h
@@ -67,15 +67,30 @@ class LLCond
LLCond(const LLCond&) = delete;
LLCond& operator=(const LLCond&) = delete;
- /// get() returns the stored DATA by value -- so to use get(), DATA must
- /// be copyable. The only way to get a non-const reference -- to modify
- /// the stored DATA -- is via update_one() or update_all().
+ /**
+ * get() returns the stored DATA by value -- so to use get(), DATA must
+ * be copyable. The only way to get a non-const reference -- to modify
+ * the stored DATA -- is via update_one() or update_all().
+ */
value_type get()
{
LockType lk(mMutex);
return mData;
}
+ /**
+ * get(functor) returns whatever the functor returns. It allows us to peek
+ * at the stored DATA without copying the whole thing. The functor must
+ * accept a const reference to DATA. If you want to modify DATA, call
+ * update_one() or update_all() instead.
+ */
+ template <typename FUNC>
+ auto get(FUNC&& func)
+ {
+ LockType lk(mMutex);
+ return std::forward<FUNC>(func)(const_data());
+ }
+
/**
* Pass update_one() an invocable accepting non-const (DATA&). The
* invocable will presumably modify the referenced DATA. update_one()
@@ -86,11 +101,11 @@ class LLCond
* update_one() when DATA is a struct or class.
*/
template <typename MODIFY>
- void update_one(MODIFY modify)
+ void update_one(MODIFY&& modify)
{
{ // scope of lock can/should end before notify_one()
LockType lk(mMutex);
- modify(mData);
+ std::forward<MODIFY>(modify)(mData);
}
mCond.notify_one();
}
@@ -105,11 +120,11 @@ class LLCond
* update_all() when DATA is a struct or class.
*/
template <typename MODIFY>
- void update_all(MODIFY modify)
+ void update_all(MODIFY&& modify)
{
{ // scope of lock can/should end before notify_all()
LockType lk(mMutex);
- modify(mData);
+ std::forward<MODIFY>(modify)(mData);
}
mCond.notify_all();
}
@@ -122,7 +137,7 @@ class LLCond
* wait() on the condition_variable.
*/
template <typename Pred>
- void wait(Pred pred)
+ void wait(Pred&& pred)
{
LockType lk(mMutex);
// We must iterate explicitly since the predicate accepted by
@@ -133,7 +148,7 @@ class LLCond
// But what if they instead pass a predicate accepting non-const
// (DATA&)? Such a predicate could modify mData, which would be Bad.
// Forbid that.
- while (! pred(const_cast<const value_type&>(mData)))
+ while (! std::forward<Pred>(pred)(const_data()))
{
mCond.wait(lk);
}
@@ -150,7 +165,7 @@ class LLCond
* returning true.
*/
template <typename Rep, typename Period, typename Pred>
- bool wait_for(const std::chrono::duration<Rep, Period>& timeout_duration, Pred pred)
+ bool wait_for(const std::chrono::duration<Rep, Period>& timeout_duration, Pred&& pred)
{
// Instead of replicating wait_until() logic, convert duration to
// time_point and just call wait_until().
@@ -159,7 +174,8 @@ class LLCond
// wrong! We'd keep pushing the timeout time farther and farther into
// the future. This way, we establish a definite timeout time and
// stick to it.
- return wait_until(std::chrono::steady_clock::now() + timeout_duration, pred);
+ return wait_until(std::chrono::steady_clock::now() + timeout_duration,
+ std::forward<Pred>(pred));
}
/**
@@ -169,9 +185,9 @@ class LLCond
* generic wait_for() method.
*/
template <typename Pred>
- bool wait_for(F32Milliseconds timeout_duration, Pred pred)
+ bool wait_for(F32Milliseconds timeout_duration, Pred&& pred)
{
- return wait_for(convert(timeout_duration), pred);
+ return wait_for(convert(timeout_duration), std::forward<Pred>(pred));
}
protected:
@@ -189,6 +205,10 @@ class LLCond
}
private:
+ // It's important to pass a const ref to certain user-specified functors
+ // that aren't supposed to be able to modify mData.
+ const value_type& const_data() const { return mData; }
+
/**
* Pass wait_until() a chrono::time_point, indicating the time at which we
* should stop waiting, and a predicate accepting (const DATA&), returning
@@ -209,21 +229,21 @@ class LLCond
* honoring a fixed timeout.
*/
template <typename Clock, typename Duration, typename Pred>
- bool wait_until(const std::chrono::time_point<Clock, Duration>& timeout_time, Pred pred)
+ bool wait_until(const std::chrono::time_point<Clock, Duration>& timeout_time, Pred&& pred)
{
LockType lk(mMutex);
// We advise the caller to pass a predicate accepting (const DATA&).
// But what if they instead pass a predicate accepting non-const
// (DATA&)? Such a predicate could modify mData, which would be Bad.
// Forbid that.
- while (! pred(const_cast<const value_type&>(mData)))
+ while (! std::forward<Pred>(pred)(const_data()))
{
if (cv_status::timeout == mCond.wait_until(lk, timeout_time))
{
// It's possible that wait_until() timed out AND the predicate
// became true more or less simultaneously. Even though
// wait_until() timed out, check the predicate one more time.
- return pred(const_cast<const value_type&>(mData));
+ return std::forward<Pred>(pred)(const_data());
}
}
return true;
diff --git a/indra/llcommon/tests/workqueue_test.cpp b/indra/llcommon/tests/workqueue_test.cpp
index bea3ad911b0..1d73f7aa0d4 100644
--- a/indra/llcommon/tests/workqueue_test.cpp
+++ b/indra/llcommon/tests/workqueue_test.cpp
@@ -99,9 +99,15 @@ namespace tut
return (++count < 3);
});
// no convenient way to close() our queue while we've got a
- // postEvery() running, so run until we think we should have exhausted
- // the iterations
- queue.runFor(10*interval);
+ // postEvery() running, so run until we have exhausted the iterations
+ // or we time out waiting
+ for (auto finish = start + 10*interval;
+ WorkQueue::TimePoint::clock::now() < finish &&
+ data.get([](const Shared& data){ return data.size(); }) < 3; )
+ {
+ queue.runPending();
+ std::this_thread::sleep_for(interval/10);
+ }
// Take a copy of the captured deque.
Shared result = data.get();
ensure_equals("called wrong number of times", result.size(), 3);
--
GitLab