Merge pull request #5756 from johnhaddon/dispatcherSignalsShutdown

More Python/Signal shutdown crash fixes
GafferHQ · Mar 28, 2024 · a9d466c · a9d466c
2 parents fd2095f + 0f080ce
commit a9d466c
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 16 deletions.
diff --git a/Changes.md b/Changes.md
@@ -5,7 +5,9 @@ Fixes
 -----
 
 - GafferTest, GafferImageTest : Fixed import of these modules if the `Gaffer` module had not been imported previously.
-- SceneAlgo : Fixed potential shutdown crashes caused by the adaptor registry.
+- SceneAlgo : Fixed potential shutdown crashes caused by the adaptor registry [^1].
+- Dispatcher : Fixed shutdown crashes caused by Python slots connected to the dispatch signals [^1].
+- Display : Fixed shutdown crashes caused by Python slots connected to `driverCreatedSignal()` and `imageReceivedSignal()` [^1].
 
 1.4.0.0b5 (relative to 1.4.0.0b4)
 =========

diff --git a/include/GafferDispatch/Dispatcher.h b/include/GafferDispatch/Dispatcher.h
@@ -296,9 +296,6 @@ class GAFFERDISPATCH_API Dispatcher : public TaskNode
 		class Batcher;
 
 		static size_t g_firstPlugIndex;
-		static PreDispatchSignal g_preDispatchSignal;
-		static DispatchSignal g_dispatchSignal;
-		static PostDispatchSignal g_postDispatchSignal;
 		static std::string g_defaultDispatcherType;
 
 };

diff --git a/python/GafferDispatchTest/DispatcherTest.py b/python/GafferDispatchTest/DispatcherTest.py
@@ -36,6 +36,7 @@
 
 import os
 import stat
+import subprocess
 import unittest
 import functools
 import itertools
@@ -2295,5 +2296,22 @@ def testPostDispatchSignalSuccess( self ) :
 		self.assertEqual( len( postDispatchSlot ), 4 )
 		self.assertEqual( postDispatchSlot[3], ( script["dispatcher"], False ) )
 
+	def testDispatchSignalShutdownCrash( self ) :
+
+		subprocess.check_call( [
+			Gaffer.executablePath(), "env", "python", "-c",
+			"""import GafferDispatch; GafferDispatch.Dispatcher.preDispatchSignal().connect( lambda d : True, scoped = False )"""
+		] )
+
+		subprocess.check_call( [
+			Gaffer.executablePath(), "env", "python", "-c",
+			"""import GafferDispatch; GafferDispatch.Dispatcher.dispatchSignal().connect( lambda d : None, scoped = False )"""
+		] )
+
+		subprocess.check_call( [
+			Gaffer.executablePath(), "env", "python", "-c",
+			"""import GafferDispatch; GafferDispatch.Dispatcher.postDispatchSignal().connect( lambda d, s : None, scoped = False )"""
+		] )
+
 if __name__ == "__main__":
 	unittest.main()
diff --git a/python/GafferImage/__init__.py b/python/GafferImage/__init__.py
@@ -34,9 +34,9 @@
 #
 ##########################################################################
 
-__import__( "IECoreImage" )
-__import__( "Gaffer" )
 __import__( "GafferDispatch" )
+__import__( "Gaffer" )
+__import__( "IECoreImage" )
 
 from ._GafferImage import *
 from .CatalogueSelect import CatalogueSelect

diff --git a/python/GafferImageTest/DisplayTest.py b/python/GafferImageTest/DisplayTest.py
@@ -319,6 +319,18 @@ def testSetDriver( self ) :
 			driver.close()
 			self.assertTrue( display.driverClosed() )
 
+	def testSignalShutdownCrash( self ) :
+
+		subprocess.check_call( [
+			Gaffer.executablePath(), "env", "python", "-c",
+			"""import GafferImage; GafferImage.Display.driverCreatedSignal().connect( lambda d, p : None, scoped = False )"""
+		] )
+
+		subprocess.check_call( [
+			Gaffer.executablePath(), "env", "python", "-c",
+			"""import GafferImage; GafferImage.Display.imageReceivedSignal().connect( lambda p : None, scoped = False )"""
+		] )
+
 	def __testTransferImage( self, fileName ) :
 
 		imageReader = GafferImage.ImageReader()

diff --git a/src/GafferDispatch/Dispatcher.cpp b/src/GafferDispatch/Dispatcher.cpp
@@ -167,9 +167,6 @@ const InternedString g_frameRangeEnd( "frameRange:end" );
 } // namespace
 
 size_t Dispatcher::g_firstPlugIndex = 0;
-Dispatcher::PreDispatchSignal Dispatcher::g_preDispatchSignal;
-Dispatcher::DispatchSignal Dispatcher::g_dispatchSignal;
-Dispatcher::PostDispatchSignal Dispatcher::g_postDispatchSignal;
 std::string Dispatcher::g_defaultDispatcherType = "";
 
 GAFFER_NODE_DEFINE_TYPE( Dispatcher )
@@ -325,17 +322,20 @@ void Dispatcher::createJobDirectory( const Gaffer::ScriptNode *script, Gaffer::C
 
 Dispatcher::PreDispatchSignal &Dispatcher::preDispatchSignal()
 {
-	return g_preDispatchSignal;
+	static PreDispatchSignal *g_preDispatchSignal = new PreDispatchSignal;
+	return *g_preDispatchSignal;
 }
 
 Dispatcher::DispatchSignal &Dispatcher::dispatchSignal()
 {
-	return g_dispatchSignal;
+	static DispatchSignal *g_dispatchSignal = new DispatchSignal;
+	return *g_dispatchSignal;
 }
 
 Dispatcher::PostDispatchSignal &Dispatcher::postDispatchSignal()
 {
-	return g_postDispatchSignal;
+	static PostDispatchSignal *g_postDispatchSignal = new PostDispatchSignal;
+	return *g_postDispatchSignal;
 }
 
 void Dispatcher::setupPlugs( Plug *parentPlug )

diff --git a/src/GafferImage/Display.cpp b/src/GafferImage/Display.cpp
@@ -466,14 +466,14 @@ void Display::affects( const Gaffer::Plug *input, AffectedPlugsContainer &output
 
 Display::DriverCreatedSignal &Display::driverCreatedSignal()
 {
-	static DriverCreatedSignal s;
-	return s;
+	static DriverCreatedSignal *g_driverCreatedSignal = new DriverCreatedSignal;
+	return *g_driverCreatedSignal;
 }
 
 Node::UnaryPlugSignal &Display::imageReceivedSignal()
 {
-	static UnaryPlugSignal s;
-	return s;
+	static UnaryPlugSignal *g_imageReceivedSignal = new UnaryPlugSignal;
+	return *g_imageReceivedSignal;
 }
 
 void Display::setDriver( IECoreImage::DisplayDriverPtr driver, bool copy )