Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop swallowing exceptions #58

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Qqwy
Copy link

@Qqwy Qqwy commented Dec 10, 2024

Fixes #57

This change ensure that rather calling PyErr.print() which prints the exception and then forgets about it, we call PyErr.restore() which restores it as 'current' exception.

If the calling Rust code:

  • Returns a (), this will now trigger the same exception once we reach Python
  • Uses PyErr::take to check for a current exception, it will fetch the exception and is able to manipulate it and turn it into a PyResult in whichever way it sees fit.
  • Doesn't use PyErr::take to defuse the exception and returns something other than () itself, then Python will turn the originally thrown exception into a SystemError with the message SystemError: <method 'example' of 'builtins.YourCustomClass' objects> returned a result with an exception set. The original exception is included as cause of this exception.

Copy link
Owner

@vorner vorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, thank you :-).

Just one question. Did you try what happens if both there's a pre-existing exception and one is raised during the logging? Will it overwrite it, or will there be some kind of „Multiple exceptions“ thing.

@Qqwy
Copy link
Author

Qqwy commented Dec 15, 2024

The current implementation will keep the pre-existing exception intact, i. e. "first exception wins".

@Qqwy
Copy link
Author

Qqwy commented Dec 16, 2024

I ran cargo fmt now to ensure the linting workflow passes.

The other workflows seem to not be specific to this branch; they seem to be failing when fetching Python versions and the like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: The logger swallows exceptions (such as KeyboardInterrupt)
2 participants