diff --git a/Changes.md b/Changes.md index bbd3fd8a666..d6ba93c07a2 100644 --- a/Changes.md +++ b/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 --- diff --git a/python/GafferUI/Editor.py b/python/GafferUI/Editor.py index ed62d94be45..1cf382efa4e 100644 --- a/python/GafferUI/Editor.py +++ b/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 diff --git a/python/GafferUITest/EditorTest.py b/python/GafferUITest/EditorTest.py index 935cbfaba47..38c472e2a63 100644 --- a/python/GafferUITest/EditorTest.py +++ b/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() diff --git a/src/Gaffer/BackgroundTask.cpp b/src/Gaffer/BackgroundTask.cpp index 9d88197ed72..fb4f47ac4a1 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,11 @@ 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", "Deadlock detected : Task is attempting to wait for itself. Please provide stack trace in bug report." ); + } + m_taskData->conditionVariable.wait( lock, [this]{