Harden Python bindings against shutdown-time destructor errors #234
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses interpreter shutdown issues in Python bindings by adding a safety guard to prevent NoneType errors during teardown, fixing issue #226. The changes ensure that destructor calls during Python interpreter shutdown don't fail when the ErrorCodes module has already been cleared.
Key Changes:
- Modified the
checkErrormethod in Python bindings to check ifErrorCodesis available before accessing it - Updated interface GUIDs in Pascal implementation files
- Corrected class inheritance declarations in WASM bindings
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/buildbindingpython.go | Added shutdown-safe guard to generated Python wrapper's checkError method to handle interpreter teardown |
| Examples/RTTI/RTTI_component/Bindings/Python/RTTI.py | Applied the generated shutdown-safe guard to example Python bindings |
| Examples/RTTI/RTTI_component/Implementations/Pascal/Interfaces/rtti_interfaces.pas | Updated interface GUIDs for Pascal implementation |
| Examples/RTTI/RTTI_component/Examples/Python/build.sh | Added Python binary location check to build script |
| Examples/RTTI/RTTI_component/Bindings/WASM/rtti_bindings.cpp | Fixed missing base class declarations and updated ACT version comment |
| Examples/RTTI/RTTI_component/Bindings/CSharp/RTTI.cs | Renamed exception class from RTTIException to ERTTIException for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Exception class for RTTI errors | ||
| /// </summary> | ||
| public class RTTIException : Exception | ||
| public class ERTTIException : Exception |
There was a problem hiding this comment.
The exception class is renamed from RTTIException to ERTTIException, but this inconsistency should be resolved. The class name ERTTIException doesn't follow C# naming conventions, which typically don't prefix exception classes with 'E'. Consider using RttiException instead to maintain consistency with .NET naming standards.
| if errorCode != ErrorCodes.SUCCESS.value: | ||
| ec = globals().get('ErrorCodes') | ||
| if ec is None: | ||
| # Interpreter shutdown: ErrorCodes may already be cleared; avoid noisy teardown |
There was a problem hiding this comment.
The word 'noisy' in the comment could be clearer. Consider rephrasing to 'avoid errors during teardown' for better clarity.
| # Interpreter shutdown: ErrorCodes may already be cleared; avoid noisy teardown | |
| # Interpreter shutdown: ErrorCodes may already be cleared; avoid errors during teardown |
2 participants
This PR fixes #226