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.

2 files changed, 45 insertions, 19 deletions

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,9 +67,11 @@ 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);
@@ -77,6 +79,19 @@ public:
     }
 
     /**
+     * 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()
      * will lock the mutex, call the invocable and then call notify_one() on
@@ -86,11 +101,11 @@ public:
      * 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 @@ public:
      * 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 @@ public:
      * 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 @@ 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<const value_type&>(mData)))
+        while (! std::forward<Pred>(pred)(const_data()))
         {
             mCond.wait(lk);
         }
@@ -150,7 +165,7 @@ public:
      * 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 @@ 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>(pred));
     }
 
     /**
@@ -169,9 +185,9 @@ public:
      * 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 @@ 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 <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 bea3ad911b..1d73f7aa0d 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);