From 6e06d1db6045df2e4961243f379c4d7695a8190d Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 7 Oct 2021 15:32:19 -0400 Subject: SL-16024: Make LLCond::get() lock and return by value. Its previous behavior, returning a const reference without locking, was wrong: it could return a reference to an object in an inconsistent state if it was concurrently being modified on another thread. Locking the mutex and returning a copy by value is the correct behavior. --- indra/llcommon/llcond.h | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) (limited to 'indra/llcommon/llcond.h') diff --git a/indra/llcommon/llcond.h b/indra/llcommon/llcond.h index e31b67d893..c08acb66a1 100644 --- a/indra/llcommon/llcond.h +++ b/indra/llcommon/llcond.h @@ -53,6 +53,8 @@ private: LLCoros::Mutex mMutex; // Use LLCoros::ConditionVariable for the same reason. LLCoros::ConditionVariable mCond; + using LockType = LLCoros::LockType; + using cv_status = LLCoros::cv_status; public: /// LLCond can be explicitly initialized with a specific value for mData if @@ -65,10 +67,14 @@ public: LLCond(const LLCond&) = delete; LLCond& operator=(const LLCond&) = delete; - /// get() returns a const reference to the stored DATA. The only way to - /// get a non-const reference -- to modify the stored DATA -- is via - /// update_one() or update_all(). - const value_type& get() const { return mData; } + /// 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; + } /** * Pass update_one() an invocable accepting non-const (DATA&). The @@ -83,7 +89,7 @@ public: void update_one(MODIFY modify) { { // scope of lock can/should end before notify_one() - LLCoros::LockType lk(mMutex); + LockType lk(mMutex); modify(mData); } mCond.notify_one(); @@ -102,7 +108,7 @@ public: void update_all(MODIFY modify) { { // scope of lock can/should end before notify_all() - LLCoros::LockType lk(mMutex); + LockType lk(mMutex); modify(mData); } mCond.notify_all(); @@ -118,7 +124,7 @@ public: template void wait(Pred pred) { - LLCoros::LockType lk(mMutex); + LockType lk(mMutex); // We must iterate explicitly since the predicate accepted by // condition_variable::wait() requires a different signature: // condition_variable::wait() calls its predicate with no arguments. @@ -205,14 +211,14 @@ private: template bool wait_until(const std::chrono::time_point& timeout_time, Pred pred) { - LLCoros::LockType lk(mMutex); + 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(mData))) { - if (LLCoros::cv_status::timeout == mCond.wait_until(lk, timeout_time)) + 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 -- cgit v1.2.3 From 04ebc11a2d8a2e59abda5061e35e504fc30504d2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 24 Nov 2021 12:56:48 -0500 Subject: 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 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 16 deletions(-) (limited to 'indra/llcommon/llcond.h') diff --git a/indra/llcommon/llcond.h b/indra/llcommon/llcond.h index c08acb66a1..da6e6affe1 100644 --- a/indra/llcommon/llcond.h +++ b/indra/llcommon/llcond.h @@ -67,15 +67,30 @@ public: 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 + auto get(FUNC&& func) + { + LockType lk(mMutex); + return std::forward(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 @@ public: * update_one() when DATA is a struct or class. */ template - 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)(mData); } mCond.notify_one(); } @@ -105,11 +120,11 @@ public: * update_all() when DATA is a struct or class. */ template - 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)(mData); } mCond.notify_all(); } @@ -122,7 +137,7 @@ public: * wait() on the condition_variable. */ template - void wait(Pred pred) + void wait(Pred&& pred) { LockType lk(mMutex); // We must iterate explicitly since the predicate accepted by @@ -133,7 +148,7 @@ public: // 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(mData))) + while (! std::forward(pred)(const_data())) { mCond.wait(lk); } @@ -150,7 +165,7 @@ public: * returning true. */ template - bool wait_for(const std::chrono::duration& timeout_duration, Pred pred) + bool wait_for(const std::chrono::duration& timeout_duration, Pred&& pred) { // Instead of replicating wait_until() logic, convert duration to // time_point and just call wait_until(). @@ -159,7 +174,8 @@ public: // 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)); } /** @@ -169,9 +185,9 @@ public: * generic wait_for() method. */ template - 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)); } protected: @@ -189,6 +205,10 @@ protected: } 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 @@ private: * honoring a fixed timeout. */ template - bool wait_until(const std::chrono::time_point& timeout_time, Pred pred) + bool wait_until(const std::chrono::time_point& 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(mData))) + while (! std::forward(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(mData)); + return std::forward(pred)(const_data()); } } return true; -- cgit v1.2.3