Skip to content

[3.6] bpo-23890: Fix ref cycles in TestCase.assertRaises() #858

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

Merged
merged 1 commit into from
Jun 15, 2017
Merged

[3.6] bpo-23890: Fix ref cycles in TestCase.assertRaises() #858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions Lib/unittest/case.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,28 +153,32 @@ def handle(self, name, args, kwargs):
If args is not empty, call a callable passing positional and keyword
arguments.
"""
if not _is_subtype(self.expected, self._base_type):
raise TypeError('%s() arg 1 must be %s' %
(name, self._base_type_str))
if args and args[0] is None:
warnings.warn("callable is None",
DeprecationWarning, 3)
args = ()
if not args:
self.msg = kwargs.pop('msg', None)
if kwargs:
warnings.warn('%r is an invalid keyword argument for '
'this function' % next(iter(kwargs)),
DeprecationWarning, 3)
return self

callable_obj, *args = args
try:
self.obj_name = callable_obj.__name__
except AttributeError:
self.obj_name = str(callable_obj)
with self:
callable_obj(*args, **kwargs)
if not _is_subtype(self.expected, self._base_type):
raise TypeError('%s() arg 1 must be %s' %
(name, self._base_type_str))
if args and args[0] is None:
warnings.warn("callable is None",
DeprecationWarning, 3)
args = ()
if not args:
self.msg = kwargs.pop('msg', None)
if kwargs:
warnings.warn('%r is an invalid keyword argument for '
'this function' % next(iter(kwargs)),
DeprecationWarning, 3)
return self

callable_obj, *args = args
try:
self.obj_name = callable_obj.__name__
except AttributeError:
self.obj_name = str(callable_obj)
with self:
callable_obj(*args, **kwargs)
finally:
# bpo-23890: manually break a reference cycle
self = None


class _AssertRaisesContext(_AssertRaisesBaseContext):
Expand Down Expand Up @@ -725,7 +729,11 @@ def assertRaises(self, expected_exception, *args, **kwargs):
self.assertEqual(the_exception.error_code, 3)
"""
context = _AssertRaisesContext(expected_exception, self)
return context.handle('assertRaises', args, kwargs)
try:
return context.handle('assertRaises', args, kwargs)
finally:
# bpo-23890: manually break a reference cycle
context = None

def assertWarns(self, expected_warning, *args, **kwargs):
"""Fail unless a warning of class warnClass is triggered
Expand Down
13 changes: 13 additions & 0 deletions Lib/unittest/test/test_case.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,19 @@ def testAssertRaisesNoExceptionType(self):
with self.assertRaises(TypeError):
self.assertRaises((ValueError, object))

def testAssertRaisesRefcount(self):
Copy link

Choose a reason for hiding this comment

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

Given the new try/finally handling and the potential for unexpected behavior, I think the context manager mode needs to be checked as well. I don't see a need for a separate test function, just add something like:

with self.assertRaises(ValueError):
    func()

self.assertEqual(refcount, sys.getrefcount(func))

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any reference cycle when using the context manager protocol.

It seems like bpo-23890 was very specific to the function API: assertRaises(ValueError, func) since you pass func to assertRaises().

with self.assertRaises(ValueError):
    func()

Here func variable may also leak in the traceback objects linked to the raisen exception. But again, I'm unable to see any leak here without my fix.

If you still consider that an unit test is needed, please comment http://bugs.python.org/issue23890

# bpo-23890: assertRaises() must not keep objects alive longer
# than expected
def func() :
try:
raise ValueError
except ValueError:
raise ValueError

refcount = sys.getrefcount(func)
self.assertRaises(ValueError, func)
self.assertEqual(refcount, sys.getrefcount(func))

def testAssertRaisesRegex(self):
class ExceptionMock(Exception):
pass
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ Core and Builtins
Library
-------

- bpo-23890: unittest.TestCase.assertRaises() now manually breaks a reference
cycle to not keep objects alive longer than expected.

- bpo-30149: inspect.signature() now supports callables with
variable-argument parameters wrapped with partialmethod.
Patch by Dong-hee Na.
Expand Down