BackgroundTask : Mitigate against attempts to wait on self

This would also have avoided the deadlock fixed in the previous commit. I'm not sure if its worth keeping or not. On the one hand, it would identify similar problems in future without exposing the user to hangs. But on the other hand, the warning makes it harder to trace the exact source of the problem, whereas a hung process makes that relatively easy with `eu-stack`.

Changes.md

@@ -17,6 +17,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)
 =======

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()

src/Gaffer/BackgroundTask.cpp

@@ -50,6 +50,8 @@
 
 #include "fmt/format.h"
 
+#include <thread>
+
 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<std::mutex> 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]{