Merge pull request #1397 from johnhaddon/exceptionAlgoFix

ExceptionAlgo : Fix `translatePythonException()` reference counting bug

Author: johnhaddon

Changes

@@ -1,7 +1,10 @@
 10.5.x.x (relative to 10.5.4.1)
 ========
 
+Fixes
+-----
 
+- ExceptionAlgo : Fixed memory leak in `translatePythonException()`, which was failing to manage the reference count for exceptions bound by `IECorePython::ExceptionClass`.
 
 10.5.4.1 (relative to 10.5.4.0)
 ========

src/IECorePython/ExceptionAlgo.cpp

@@ -46,30 +46,23 @@ namespace
 {
 
 std::string formatInternal(
-	PyObject *exceptionPyObject, PyObject *valuePyObject, PyObject *tracebackPyObject,
+	const handle<> &exceptionHandle, const handle<> &valueHandle, const handle<> &tracebackHandle,
 	bool withStacktrace, int *lineNumber = nullptr
 )
 {
-	if( !exceptionPyObject )
-	{
-		throw IECore::Exception( "No Python exception set" );
-	}
-
-	PyErr_NormalizeException( &exceptionPyObject, &valuePyObject, &tracebackPyObject );
+	object exception( exceptionHandle );
 
-	object exception( ( handle<>( exceptionPyObject ) ) );
-
-	// valuePyObject and tracebackPyObject may be null.
+	// `valueHandle` and `tracebackHandle` may be null.
 	object value;
-	if( valuePyObject )
+	if( valueHandle )
 	{
-		value = object( handle<>( valuePyObject ) );
+		value = object( valueHandle );
 	}
 
 	object traceback;
-	if( tracebackPyObject )
+	if( tracebackHandle )
 	{
-		traceback = object( handle<>( tracebackPyObject ) );
+		traceback = object( tracebackHandle );
 	}
 
 	if( lineNumber )
@@ -102,6 +95,20 @@ std::string formatInternal(
 	return s;
 }
 
+std::tuple<handle<>, handle<>, handle<>> currentPythonException()
+{
+	PyObject *exceptionPyObject, *valuePyObject, *tracebackPyObject;
+	PyErr_Fetch( &exceptionPyObject, &valuePyObject, &tracebackPyObject );
+	if( !exceptionPyObject )
+	{
+		throw IECore::Exception( "No Python exception set" );
+	}
+
+	PyErr_NormalizeException( &exceptionPyObject, &valuePyObject, &tracebackPyObject );
+
+	return std::make_tuple( handle<>( exceptionPyObject ), handle<>( allow_null( valuePyObject ) ), handle<>( allow_null( tracebackPyObject ) ) );
+}
+
 } // namespace
 
 namespace IECorePython
@@ -112,28 +119,26 @@ namespace ExceptionAlgo
 
 std::string formatPythonException( bool withStacktrace, int *lineNumber )
 {
-	PyObject *exceptionPyObject, *valuePyObject, *tracebackPyObject;
-	PyErr_Fetch( &exceptionPyObject, &valuePyObject, &tracebackPyObject );
-	return formatInternal( exceptionPyObject, valuePyObject, tracebackPyObject, withStacktrace, lineNumber );
+	auto [exception, value, traceback] = currentPythonException();
+	return formatInternal( exception, value, traceback, withStacktrace, lineNumber );
 }
 
 void translatePythonException( bool withStacktrace )
 {
-	PyObject *exceptionPyObject, *valuePyObject, *tracebackPyObject;
-	PyErr_Fetch( &exceptionPyObject, &valuePyObject, &tracebackPyObject );
+	auto [exception, value, traceback] = currentPythonException();
 
 	// If the python exception is one bound via IECorePython::ExceptionClass,
 	// then we can extract and throw the C++ exception held internally.
-	if( PyObject_HasAttrString( valuePyObject, "__exceptionPointer" ) )
+	if( PyObject_HasAttrString( value.get(), "__exceptionPointer" ) )
 	{
-		object exceptionPointerMethod( handle<>( PyObject_GetAttrString( valuePyObject, "__exceptionPointer" ) ) );
+		object exceptionPointerMethod( handle<>( PyObject_GetAttrString( value.get(), "__exceptionPointer" ) ) );
 		object exceptionPointerObject = exceptionPointerMethod();
 		std::exception_ptr exceptionPointer = boost::python::extract<std::exception_ptr>( exceptionPointerObject )();
 		std::rethrow_exception( exceptionPointer );
 	}
 
 	// Otherwise, we just throw a generic exception describing the python error.
-	throw IECore::Exception( formatInternal( exceptionPyObject, valuePyObject, tracebackPyObject, withStacktrace ) );
+	throw IECore::Exception( formatInternal( exception, value, traceback, withStacktrace ) );
 }
 
 } // namespace ExceptionAlgo

test/IECore/MessageHandler.py

@@ -33,6 +33,7 @@
 ##########################################################################
 
 from __future__ import with_statement
+import gc
 import unittest
 import threading
 import time
@@ -224,5 +225,46 @@ def testLifetime( self ) :
 
 		self.assertEqual( w(), None )
 
+	def testExceptionHandling( self ) :
+
+		# This is more a test of ExceptionAlgo than it is MessageHandler, but
+		# this is the most convenient place to put the test right now.
+
+		class ThrowingMessageHandler( IECore.MessageHandler ):
+
+			def __init__( self ) :
+
+				IECore.MessageHandler.__init__( self )
+
+			def handle( self, level, context, msg ):
+
+				if context == "python" :
+					raise Exception( "Test" )
+				else :
+					assert( context == "c++" )
+					# This will raise a C++ exception that gets translated to a
+					# Python exception, and then gets translated back to a C++
+					# exception by the MessageHandlerWrapper, before finally
+					# being converted back to another Python exception by the
+					# binding for `IECore.msg()`.
+					IECore.StringAlgo.substitute( "##", { "frame" : IECore.BoolData( False ) } )
+
+		for exceptionType in [ "python", "c++" ] :
+
+			with self.subTest( exceptionType = exceptionType ) :
+
+				while gc.collect() :
+					pass
+
+				expected = "Test" if exceptionType == "python" else "Unexpected data type"
+				with self.assertRaisesRegex( Exception, expected ) :
+					with ThrowingMessageHandler() :
+						IECore.msg( IECore.Msg.Level.Error, exceptionType, "message" )
+
+				# There should be no Exception objects in the garbage pool. If
+				# there were, that would indicate a reference counting error in
+				# `ExceptionAlgo::translatePythonException()`.
+				self.assertEqual( [ o for o in gc.get_objects() if isinstance( o, Exception ) ], [] )
+
 if __name__ == "__main__":
     unittest.main()