Skip to content

Fix exception refcnt #1402

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Fix exception refcnt #1402

wants to merge 6 commits into from

Conversation

BadSingleton
Copy link
Contributor

@BadSingleton BadSingleton commented Mar 1, 2021 •
edited by filmor
Loading

What does this implement/fix? Explain your changes.

Adds a tp_dealloc function to ExceptionClassObject, so we clean the BaseException members the same way as other the derived exceptions in the cpython code does.

Does this close any currently open issues?

Fixes issue #1371.

Any other comments?

Fixes a bug where C# Exceptions set as the Inner of another C# Exception, then converted into a Python exception object (and thus having __cause__ set to a valid object) would leak the Inner/__cause__ objects.

Simplified diagram :

AggregateException.Inner        -> ArgumentException(...)
      ^                                    ^
      |                                    |
PyAggregateException.__cause__  -> PyArgumentException

In this case, where the AggregateException would be thrown from C# and caught in Python, the AggregateException and PyAggregateException would be freed, but not the ArgumentException set as the cause.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@BadSingleton
Fix a ref count issue in PythonException
ee30c94
@BadSingleton
Fixes a leak in ExceptionClassObject
480a549 
As seen in exceptions.c (from cpython), derived exception classes must
also clean the ExceptionBase members. If we don't, we leak the Python
and C# objects set as the __cause__/Inner (amongst
others) of thrown C# exceptions.

This does not address the serializability of the PythonException class.

Also fix various PythonException (ab)uses.
lostmsu
lostmsu requested changes Mar 1, 2021
View reviewed changes
Copy link
Member

@lostmsu lostmsu left a comment

Choose a reason for hiding this comment

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

Mostly NITs, but Clear and Normalize must be rechecked.

lostmsu
lostmsu requested changes Mar 4, 2021
View reviewed changes
@lostmsu
Copy link
Member

lostmsu commented Mar 5, 2021

@BadSingleton can you investigate timeout in Ubuntu Py 3.6 tests?

@BadSingleton
Copy link
Contributor Author

Huh.. that's weird. I'll take a look into it.

@BadSingleton BadSingleton mentioned this pull request Mar 9, 2021
Closed
@BadSingleton
Copy link
Contributor Author

I'm blocked due to #1412, this seems like an issue with garbage collection. This does raise the question of "How much do we care about Python 3.6?" There's less than a year before it's EOL: https://www.python.org/dev/peps/pep-0494/#lifespan .

@mjmvisser
Copy link
Contributor

mjmvisser commented Sep 2, 2021 •
edited
Loading

@BadSingleton is this still blocked? (Benedikt says "this needs a rebase")

@lostmsu
Copy link
Member

lostmsu commented Sep 2, 2021

@mjmvisser @BadSingleton AFAIK you should now be able to use a debug build of Python.

@BadSingleton
Copy link
Contributor Author

Apologies for the late reply, was on vacations. Yes this needs a rebase. I hadn't had the time to take a look at it in a while, but we're planning on resuming work on this Soon ™️

@lostmsu
Copy link
Member

lostmsu commented Jan 20, 2022

@BadSingleton can you please see if the issue is still present in master, and if it is not, I would suggest to open a new PR with regression tests (you seem to have added a few here).

@BadSingleton
Copy link
Contributor Author

BadSingleton commented Jan 20, 2022 •
edited
Loading

Looks like cb4bb9a which re-enabled the test test_method_parameters_change supersedes this PR. I'll see if the other test cases are still applicable

BadSingleton added a commit to Unity-Technologies/pythonnet that referenced this pull request Jan 24, 2022
@BadSingleton
Add tests for exception leaking.
e481ab9 
Originally from PR pythonnet#1402. The underlying bug is now fixed, but the tests
are atill applicable.
@BadSingleton BadSingleton mentioned this pull request Jan 24, 2022
Merged
5 tasks
@BadSingleton
Copy link
Contributor Author

Closing. Superseded by #1679

lostmsu pushed a commit that referenced this pull request Jan 25, 2022
@BadSingleton @lostmsu
Add tests for exception leaking.
ca1a72b 
Originally from PR #1402. The underlying bug is now fixed, but the tests
are atill applicable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmor filmor filmor left review comments

@siebenmann siebenmann siebenmann left review comments

@lostmsu lostmsu lostmsu requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BadSingleton @lostmsu @mjmvisser @filmor @siebenmann