Editor : Fix deadlock caused by garbage collection of Settings

Changes.md

@@ -19,12 +19,18 @@ Improvements
 - SceneInspector :
   - Added support for dragging inspector labels, such as those containing the names of attributes, options, output parameters, parameters, primitive variables, and sets.
   - Set names beginning with "__" such as "__lights" or "__cameras" are now displayed as-is, rather than being transformed to "Lights" or "Cameras".
+- BackgroundTask : Added reporting of tasks attempting to wait for themselves.
 
 Fixes
 -----
 
 - Viewer : Fixed handling of Gaffer `${contextVariable}` references in OpenColorIO variable values. The Viewer now updates the Display Transform appropriately when the value of the context variable changes.
 
+Fixes
+-----
+
+- UI : Fixed hangs caused by garbage collection of removed Editors. One common example involved viewing a Catalogue in the NodeEditor after removing the ImageInspector (#5877).
+
 API
 ---
 

python/GafferUI/Editor.py

@@ -96,6 +96,17 @@ def __init__( self, topLevelWidget, scriptNode, **kw ) :
 
 		self.__setContextInternal( scriptNode.context(), callUpdate=False )
 
+	def __del__( self ) :
+
+		for attr in self.__dict__.values() :
+			if isinstance( attr, GafferUI.Editor.Settings ) :
+				# Remove connection to ScriptNode now, on the UI thread.
+				# Otherwise we risk deadlock if the Settings node gets garbage
+				# collected in a BackgroundTask, which would attempt
+				# cancellation of all tasks for the ScriptNode, including the
+				# task itself.
+				attr["__scriptNode"].setInput( None )
+
 	def scriptNode( self ) :
 
 		return self.__scriptNode

python/GafferUITest/EditorTest.py

@@ -38,6 +38,8 @@
 import unittest
 import weakref
 
+import IECore
+
 import Gaffer
 import GafferTest
 import GafferUI
@@ -110,5 +112,50 @@ def pythonEditorCreated( editor ) :
 		self.assertEqual( editorsCreated, [ e1, e2, e3 ] )
 		self.assertEqual( pythonEditorsCreated, [ e2 ] )
 
+	def testIssue5877( self ) :
+
+		# This test models the bug reported in #5877. First we have
+		# an editor that uses a `Settings` node.
+
+		script = Gaffer.ScriptNode()
+		script["node"] = GafferTest.MultiplyNode()
+
+		class TestEditor( GafferUI.Editor ) :
+
+			def __init__( self, scriptNode, **kw ) :
+
+				GafferUI.Editor.__init__( self, GafferUI.Label( "MyEditor" ), scriptNode )
+
+				self.__settingsNode = self.Settings( "MyEditor", scriptNode )
+
+		editor = TestEditor( script )
+
+		# Then we dispose of that editor.
+
+		del editor
+
+		# Now we run a perfectly innocent background task that just
+		# wants to compute something perfectly innocently.
+
+		def f( canceller ) :
+
+			script["node"]["product"].getValue()
+
+			# But of course, garbage collection can occur at any time.
+			# For the purposes of this test we force it to happen explicitly,
+			# but in the real world it would be triggered by the creation
+			# of a new wrapped RefCounted instance. In #5877 this was commonly
+			# an `_ImagesPath` created by the CatalogueUI in a background task
+			# updating the PathListingWidget for the image listing.
+			IECore.RefCounted.collectGarbage()
+
+		task = Gaffer.BackgroundTask( script["node"]["product"], f )
+
+		# This would have deadlocked when `collectGarbage()` triggered the
+		# destruction of the Settings node, because that would trigger
+		# cancellation of the BackgroundTask from the task itself. And a task
+		# waiting on itself would never finish.
+		task.wait()
+
 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,11 @@ 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", "Deadlock detected : Task is attempting to wait for itself. Please provide stack trace in bug report." );
+	}
+
 	m_taskData->conditionVariable.wait(
 		lock,
 		[this]{