Merge pull request #208 from secondlife/SL-19690

SL-19690: Follow up on Rye Mutt's fix for shutdown crashes.

7 files changed, 105 insertions, 159 deletions

diff --git a/indra/llaudio/llaudiodecodemgr.cpp b/indra/llaudio/llaudiodecodemgr.cpp
index 38a6b41afe..190c5290cb 100755
--- a/indra/llaudio/llaudiodecodemgr.cpp
+++ b/indra/llaudio/llaudiodecodemgr.cpp
@@ -607,40 +607,37 @@ void LLAudioDecodeMgr::Impl::startMoreDecodes()
 
         // Kick off a decode
         mDecodes[decode_id] = LLPointer<LLVorbisDecodeState>(NULL);
-        try
-        {
-            main_queue->postTo(
-                general_queue,
-                [decode_id]() // Work done on general queue
-                {
-                    LLPointer<LLVorbisDecodeState> decode_state = beginDecodingAndWritingAudio(decode_id);
-
-                    if (!decode_state)
-                    {
-                        // Audio decode has errored
-                        return decode_state;
-                    }
+        bool posted = main_queue->postTo(
+            general_queue,
+            [decode_id]() // Work done on general queue
+            {
+                LLPointer<LLVorbisDecodeState> decode_state = beginDecodingAndWritingAudio(decode_id);
 
-                    // Disk write of decoded audio is now in progress off-thread
+                if (!decode_state)
+                {
+                    // Audio decode has errored
                     return decode_state;
-                },
-                [decode_id, this](LLPointer<LLVorbisDecodeState> decode_state) // Callback to main thread
-                mutable {
-                    if (!gAudiop)
-                    {
-                        // There is no LLAudioEngine anymore. This might happen if
-                        // an audio decode is enqueued just before shutdown.
-                        return;
-                    }
-
-                    // At this point, we can be certain that the pointer to "this"
-                    // is valid because the lifetime of "this" is dependent upon
-                    // the lifetime of gAudiop.
-
-                    enqueueFinishAudio(decode_id, decode_state);
-                });
-        }
-        catch (const LLThreadSafeQueueInterrupt&)
+                }
+
+                // Disk write of decoded audio is now in progress off-thread
+                return decode_state;
+            },
+            [decode_id, this](LLPointer<LLVorbisDecodeState> decode_state) // Callback to main thread
+            mutable {
+                if (!gAudiop)
+                {
+                    // There is no LLAudioEngine anymore. This might happen if
+                    // an audio decode is enqueued just before shutdown.
+                    return;
+                }
+
+                // At this point, we can be certain that the pointer to "this"
+                // is valid because the lifetime of "this" is dependent upon
+                // the lifetime of gAudiop.
+
+                enqueueFinishAudio(decode_id, decode_state);
+            });
+        if (! posted)
         {
             // Shutdown
             // Consider making processQueue() do a cleanup instead
diff --git a/indra/llcommon/llqueuedthread.cpp b/indra/llcommon/llqueuedthread.cpp
index 9b1de2e9a5..7da7c1e026 100644
--- a/indra/llcommon/llqueuedthread.cpp
+++ b/indra/llcommon/llqueuedthread.cpp
@@ -146,7 +146,7 @@ S32 LLQueuedThread::updateQueue(F32 max_time_ms)
         // schedule a call to threadedUpdate for every call to updateQueue
         if (!isQuitting())
         {
-            mRequestQueue.postIfOpen([=]()
+            mRequestQueue.post([=]()
                 {
                     LL_PROFILE_ZONE_NAMED_CATEGORY_THREAD("qt - update");
                     mIdleThread = FALSE;
diff --git a/indra/llcommon/workqueue.cpp b/indra/llcommon/workqueue.cpp
index 83e0216ae7..cf80ce0656 100644
--- a/indra/llcommon/workqueue.cpp
+++ b/indra/llcommon/workqueue.cpp
@@ -161,12 +161,7 @@ bool LL::WorkQueue::done()
     return mQueue.done();
 }
 
-void LL::WorkQueue::post(const Work& callable)
-{
-    mQueue.push(callable);
-}
-
-bool LL::WorkQueue::postIfOpen(const Work& callable)
+bool LL::WorkQueue::post(const Work& callable)
 {
     return mQueue.pushIfOpen(callable);
 }
@@ -215,26 +210,16 @@ bool LL::WorkSchedule::done()
     return mQueue.done();
 }
 
-void LL::WorkSchedule::post(const Work& callable)
+bool LL::WorkSchedule::post(const Work& callable)
 {
     // Use TimePoint::clock::now() instead of TimePoint's representation of
     // the epoch because this WorkSchedule may contain a mix of past-due
     // TimedWork items and TimedWork items scheduled for the future. Sift this
     // new item into the correct place.
-    post(callable, TimePoint::clock::now());
-}
-
-void LL::WorkSchedule::post(const Work& callable, const TimePoint& time)
-{
-    mQueue.push(TimedWork(time, callable));
-}
-
-bool LL::WorkSchedule::postIfOpen(const Work& callable)
-{
-    return postIfOpen(callable, TimePoint::clock::now());
+    return post(callable, TimePoint::clock::now());
 }
 
-bool LL::WorkSchedule::postIfOpen(const Work& callable, const TimePoint& time)
+bool LL::WorkSchedule::post(const Work& callable, const TimePoint& time)
 {
     return mQueue.pushIfOpen(TimedWork(time, callable));
 }
diff --git a/indra/llcommon/workqueue.h b/indra/llcommon/workqueue.h
index 5b704e7198..ec0700a718 100644
--- a/indra/llcommon/workqueue.h
+++ b/indra/llcommon/workqueue.h
@@ -83,13 +83,10 @@ namespace LL
 
         /*---------------------- fire and forget API -----------------------*/
 
-        /// fire-and-forget
-        virtual void post(const Work&) = 0;
-
         /**
          * post work, unless the queue is closed before we can post
          */
-        virtual bool postIfOpen(const Work&) = 0;
+        virtual bool post(const Work&) = 0;
 
         /**
          * post work, unless the queue is full
@@ -247,13 +244,10 @@ namespace LL
 
         /*---------------------- fire and forget API -----------------------*/
 
-        /// fire-and-forget
-        void post(const Work&) override;
-
         /**
          * post work, unless the queue is closed before we can post
          */
-        bool postIfOpen(const Work&) override;
+        bool post(const Work&) override;
 
         /**
          * post work, unless the queue is full
@@ -320,22 +314,16 @@ namespace LL
 
         /*---------------------- fire and forget API -----------------------*/
 
-        /// fire-and-forget
-        void post(const Work& callable) override;
-
-        /// fire-and-forget, but at a particular (future?) time
-        void post(const Work& callable, const TimePoint& time);
-
         /**
          * post work, unless the queue is closed before we can post
          */
-        bool postIfOpen(const Work& callable) override;
+        bool post(const Work& callable) override;
 
         /**
          * post work for a particular time, unless the queue is closed before
          * we can post
          */
-        bool postIfOpen(const Work& callable, const TimePoint& time);
+        bool post(const Work& callable, const TimePoint& time);
 
         /**
          * post work, unless the queue is full
@@ -356,7 +344,7 @@ namespace LL
          * an LLCond variant, e.g. LLOneShotCond or LLBoolCond.
          */
         template <typename Rep, typename Period, typename CALLABLE>
-        void postEvery(const std::chrono::duration<Rep, Period>& interval,
+        bool postEvery(const std::chrono::duration<Rep, Period>& interval,
                        CALLABLE&& callable);
 
     private:
@@ -417,15 +405,10 @@ namespace LL
                 // move-only callable; but naturally this statement must be
                 // the last time we reference this instance, which may become
                 // moved-from.
-                try
-                {
-                    auto target{ std::dynamic_pointer_cast<WorkSchedule>(mTarget.lock()) };
-                    target->post(std::move(*this), mStart);
-                }
-                catch (const Closed&)
-                {
-                    // Once this queue is closed, oh well, just stop
-                }
+                auto target{ std::dynamic_pointer_cast<WorkSchedule>(mTarget.lock()) };
+                // Discard bool return: once this queue is closed, oh well,
+                // just stop
+                target->post(std::move(*this), mStart);
             }
         }
 
@@ -437,7 +420,7 @@ namespace LL
     };
 
     template <typename Rep, typename Period, typename CALLABLE>
-    void WorkSchedule::postEvery(const std::chrono::duration<Rep, Period>& interval,
+    bool WorkSchedule::postEvery(const std::chrono::duration<Rep, Period>& interval,
                                  CALLABLE&& callable)
     {
         if (interval.count() <= 0)
@@ -454,7 +437,7 @@ namespace LL
         // Instantiate and post a suitable BackJack, binding a weak_ptr to
         // self, the current time, the desired interval and the desired
         // callable.
-        post(
+        return post(
             BackJack<Rep, Period, CALLABLE>(
                  getWeak(), TimePoint::clock::now(), interval, std::move(callable)));
     }
@@ -516,48 +499,37 @@ namespace LL
         // Here we believe target WorkQueue still exists. Post to it a
         // lambda that packages our callable, our callback and a weak_ptr
         // to this originating WorkQueue.
-        try
-        {
-            tptr->post(
-                [reply = super::getWeak(),
-                 callable = std::move(callable),
-                 callback = std::move(callback)]
-                () mutable
+        return tptr->post(
+            [reply = super::getWeak(),
+             callable = std::move(callable),
+             callback = std::move(callback)]
+            () mutable
+            {
+                // Use postMaybe() below in case this originating WorkQueue
+                // has been closed or destroyed. Remember, the outer lambda is
+                // now running on a thread servicing the target WorkQueue, and
+                // real time has elapsed since postTo()'s tptr->post() call.
+                try
                 {
-                    // Use postMaybe() below in case this originating WorkQueue
-                    // has been closed or destroyed. Remember, the outer lambda is
-                    // now running on a thread servicing the target WorkQueue, and
-                    // real time has elapsed since postTo()'s tptr->post() call.
-                    try
-                    {
-                        // Make a reply lambda to repost to THIS WorkQueue.
-                        // Delegate to makeReplyLambda() so we can partially
-                        // specialize on void return.
-                        postMaybe(reply, makeReplyLambda(std::move(callable), std::move(callback)));
-                    }
-                    catch (...)
-                    {
-                        // Either variant of makeReplyLambda() is responsible for
-                        // calling the caller's callable. If that throws, return
-                        // the exception to the originating thread.
-                        postMaybe(
-                            reply,
-                            // Bind the current exception to transport back to the
-                            // originating WorkQueue. Once there, rethrow it.
-                            [exc = std::current_exception()](){ std::rethrow_exception(exc); });
-                    }
-                },
-                // if caller passed a TimePoint, pass it along to post()
-                std::forward<ARGS>(args)...);
-        }
-        catch (const Closed&)
-        {
-            // target WorkQueue still exists, but is Closed
-            return false;
-        }
-
-        // looks like we were able to post()
-        return true;
+                    // Make a reply lambda to repost to THIS WorkQueue.
+                    // Delegate to makeReplyLambda() so we can partially
+                    // specialize on void return.
+                    postMaybe(reply, makeReplyLambda(std::move(callable), std::move(callback)));
+                }
+                catch (...)
+                {
+                    // Either variant of makeReplyLambda() is responsible for
+                    // calling the caller's callable. If that throws, return
+                    // the exception to the originating thread.
+                    postMaybe(
+                        reply,
+                        // Bind the current exception to transport back to the
+                        // originating WorkQueue. Once there, rethrow it.
+                        [exc = std::current_exception()](){ std::rethrow_exception(exc); });
+                }
+            },
+            // if caller passed a TimePoint, pass it along to post()
+            std::forward<ARGS>(args)...);
     }
 
     template <typename... ARGS>
@@ -568,18 +540,9 @@ namespace LL
         auto tptr = target.lock();
         if (tptr)
         {
-            try
-            {
-                tptr->post(std::forward<ARGS>(args)...);
-                // we were able to post()
-                return true;
-            }
-            catch (const Closed&)
-            {
-                // target WorkQueue still exists, but is Closed
-            }
+            return tptr->post(std::forward<ARGS>(args)...);
         }
-        // either target no longer exists, or its WorkQueue is Closed
+        // target no longer exists
         return false;
     }
 
@@ -591,7 +554,7 @@ namespace LL
         auto operator()(WorkQueueBase* self, CALLABLE&& callable, ARGS&&... args)
         {
             LLCoros::Promise<RETURNTYPE> promise;
-            self->post(
+            bool posted = self->post(
                 // We dare to bind a reference to Promise because it's
                 // specifically designed for cross-thread communication.
                 [&promise, callable = std::move(callable)]()
@@ -608,6 +571,10 @@ namespace LL
                 },
                 // if caller passed a TimePoint, pass it to post()
                 std::forward<ARGS>(args)...);
+            if (! posted)
+            {
+                LLTHROW(WorkQueueBase::Closed());
+            }
             auto future{ LLCoros::getFuture(promise) };
             // now, on the calling thread, wait for that result
             LLCoros::TempStatus st("waiting for WorkQueue::waitForResult()");
@@ -623,7 +590,7 @@ namespace LL
         void operator()(WorkQueueBase* self, CALLABLE&& callable, ARGS&&... args)
         {
             LLCoros::Promise<void> promise;
-            self->post(
+            bool posted = self->post(
                 // &promise is designed for cross-thread access
                 [&promise, callable = std::move(callable)]()
                 mutable {
@@ -639,6 +606,10 @@ namespace LL
                 },
                 // if caller passed a TimePoint, pass it to post()
                 std::forward<ARGS>(args)...);
+            if (! posted)
+            {
+                LLTHROW(WorkQueueBase::Closed());
+            }
             auto future{ LLCoros::getFuture(promise) };
             // block until set_value()
             LLCoros::TempStatus st("waiting for void WorkQueue::waitForResult()");
diff --git a/indra/llimage/llimageworker.cpp b/indra/llimage/llimageworker.cpp
index 9358a0ae2c..520c81a8ec 100644
--- a/indra/llimage/llimageworker.cpp
+++ b/indra/llimage/llimageworker.cpp
@@ -92,21 +92,19 @@ LLImageDecodeThread::handle_t LLImageDecodeThread::decodeImage(
 {
     LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE;
 
-	try
-	{
-		// Instantiate the ImageRequest right in the lambda, why not?
-		mThreadPool->getQueue().post(
-			[req = ImageRequest(image, discard, needs_aux, responder)]
-			() mutable
-			{
-				auto done = req.processRequest();
-				req.finishRequest(done);
-			});
-	}
-	catch (const LLThreadSafeQueueInterrupt&)
-	{
-		LL_DEBUGS() << "Tried to start decoding on shutdown" << LL_ENDL;
-	}
+    // Instantiate the ImageRequest right in the lambda, why not?
+    bool posted = mThreadPool->getQueue().post(
+        [req = ImageRequest(image, discard, needs_aux, responder)]
+        () mutable
+        {
+            auto done = req.processRequest();
+            req.finishRequest(done);
+        });
+    if (! posted)
+    {
+        LL_DEBUGS() << "Tried to start decoding on shutdown" << LL_ENDL;
+        // should this return 0?
+    }
 
     // It's important to our consumer (LLTextureFetchWorker) that we return a
     // nonzero handle. It is NOT important that the nonzero handle be unique:
diff --git a/indra/llrender/llimagegl.h b/indra/llrender/llimagegl.h
index 87fb9e363e..243aeaea25 100644
--- a/indra/llrender/llimagegl.h
+++ b/indra/llrender/llimagegl.h
@@ -340,7 +340,7 @@ public:
     template <typename CALLABLE>
     bool post(CALLABLE&& func)
     {
-        return getQueue().postIfOpen(std::forward<CALLABLE>(func));
+        return getQueue().post(std::forward<CALLABLE>(func));
     }
 
     void run() override;
diff --git a/indra/llwindow/llwindowwin32.cpp b/indra/llwindow/llwindowwin32.cpp
index 4530e34369..1f91cbbaa8 100644
--- a/indra/llwindow/llwindowwin32.cpp
+++ b/indra/llwindow/llwindowwin32.cpp
@@ -370,15 +370,10 @@ struct LLWindowWin32::LLWindowWin32Thread : public LL::ThreadPool
     template <typename CALLABLE>
     void post(CALLABLE&& func)
     {
-        try
-        {
-            getQueue().post(std::forward<CALLABLE>(func));
-        }
-        catch (const LLThreadSafeQueueInterrupt&)
-        {
-            // Shutdown timing is tricky. The main thread can end up trying
-            // to post a cursor position after having closed the WorkQueue.
-        }
+        // Ignore bool return. Shutdown timing is tricky: the main thread can
+        // end up trying to post a cursor position after having closed the
+        // WorkQueue.
+        getQueue().post(std::forward<CALLABLE>(func));
     }
 
     /**