diff --git a/Changes.md b/Changes.md index a2636a811ed..68290067049 100644 --- a/Changes.md +++ b/Changes.md @@ -22,6 +22,7 @@ API - TextWidget, MultilineTextWidget : - Added `setPlaceholderText()` and `getPlaceholderText()` methods. - Added `placeholderText` constructor argument. +- BackgroundTask : Added mitigation against tasks attempting to wait for themselves. 1.4.6.0 (relative to 1.4.5.0) ======= diff --git a/python/GafferTest/BackgroundTaskTest.py b/python/GafferTest/BackgroundTaskTest.py index 4c8ccfbf6a2..224866ffa49 100644 --- a/python/GafferTest/BackgroundTaskTest.py +++ b/python/GafferTest/BackgroundTaskTest.py @@ -255,5 +255,24 @@ def f( canceller ) : # check for cancellation, and we'll deadlock. del task + def testSelfWait( self ) : + + def f( canceller ) : + # It makes zero sense for a task to wait for itself. Rather than + # allow it to deadlock, we want to emit a diagnostic message and + # continue. + task.wait() + + mh = IECore.CapturingMessageHandler() + IECore.MessageHandler.setDefaultHandler( mh ) + + task = Gaffer.BackgroundTask( None, f ) + task.wait() + + self.assertEqual( len( mh.messages ), 1 ) + self.assertEqual( mh.messages[0].level, IECore.Msg.Level.Error ) + self.assertEqual( mh.messages[0].context, "BackgroundTask::wait" ) + self.assertEqual( mh.messages[0].message, "Task attempted to wait for itself" ) + if __name__ == "__main__": unittest.main() diff --git a/src/Gaffer/BackgroundTask.cpp b/src/Gaffer/BackgroundTask.cpp index 9d88197ed72..c21cc7f99c7 100644 --- a/src/Gaffer/BackgroundTask.cpp +++ b/src/Gaffer/BackgroundTask.cpp @@ -50,6 +50,8 @@ #include "fmt/format.h" +#include + using namespace IECore; using namespace Gaffer; @@ -155,9 +157,10 @@ struct BackgroundTask::TaskData : public boost::noncopyable Function *function; IECore::Canceller canceller; - std::mutex mutex; // Protects `conditionVariable` and `status` + std::mutex mutex; // Protects `conditionVariable`, `status` and `threadID` std::condition_variable conditionVariable; Status status; + std::thread::id threadId; // Thread that is executing `function`, if any }; BackgroundTask::BackgroundTask( const Plug *subject, const Function &function ) @@ -186,6 +189,7 @@ BackgroundTask::BackgroundTask( const Plug *subject, const Function &function ) // Otherwise do the work. taskData->status = Running; + taskData->threadId = std::this_thread::get_id(); lock.unlock(); // Reset thread state rather then inherit the random @@ -225,6 +229,7 @@ BackgroundTask::BackgroundTask( const Plug *subject, const Function &function ) lock.lock(); taskData->status = status; + taskData->threadId = std::thread::id(); taskData->conditionVariable.notify_one(); } ); @@ -248,6 +253,12 @@ void BackgroundTask::cancel() void BackgroundTask::wait() { std::unique_lock lock( m_taskData->mutex ); + if( m_taskData->threadId == std::this_thread::get_id() ) + { + IECore::msg( IECore::Msg::Error, "BackgroundTask::wait", "Task attempted to wait for itself" ); + return; + } + m_taskData->conditionVariable.wait( lock, [this]{