diff --git a/Changes b/Changes index 99a60277f3..89a88c721f 100644 --- a/Changes +++ b/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) ======== diff --git a/src/IECorePython/ExceptionAlgo.cpp b/src/IECorePython/ExceptionAlgo.cpp index 8bd6bf5d01..67632993fb 100644 --- a/src/IECorePython/ExceptionAlgo.cpp +++ b/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<>> 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( 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 diff --git a/test/IECore/MessageHandler.py b/test/IECore/MessageHandler.py index c257b0c30e..535d9bf19a 100644 --- a/test/IECore/MessageHandler.py +++ b/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()