infinite loop when running inspect.unwrap over unittest.mock.call

[cjw296]

BPO	25532
Nosy	@ncoghlan, @cjw296, @voidspace, @takluyver, @serhiy-storchaka, @1st1, @The-Compiler, @pstch
PRs	bpo-25532: Protect against infinite loops in inspect.unwrap() #1717 [3.6] bpo-25532: Protect against infinite loops in inspect.unwrap() (GH-1717) #3778
Files	blacklist-wrapped-in-mock-call.patch: Patch blacklisting wrapped as method name in mock.call

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-09-27.09:40:40.311>
created_at = <Date 2015-11-01.23:50:17.845>
labels = ['3.7', 'type-bug', 'library']
title = 'infinite loop when running inspect.unwrap over unittest.mock.call'
updated_at = <Date 2017-09-27.09:40:40.300>
user = 'https://github.com/cjw296'

bugs.python.org fields:

activity = <Date 2017-09-27.09:40:40.300>
actor = 'ncoghlan'
assignee = 'none'
closed = True
closed_date = <Date 2017-09-27.09:40:40.311>
closer = 'ncoghlan'
components = ['Library (Lib)']
creation = <Date 2015-11-01.23:50:17.845>
creator = 'cjw296'
dependencies = []
files = ['44178']
hgrepos = []
issue_num = 25532
keywords = ['patch', '3.5regression']
message_count = 26.0
messages = ['253885', '253887', '253903', '253954', '253975', '253976', '253986', '254726', '254839', '260458', '273254', '273265', '273270', '273272', '273273', '273333', '294142', '294168', '294172', '294215', '294255', '294282', '296947', '303104', '303106', '303117']
nosy_count = 9.0
nosy_names = ['ncoghlan', 'cjw296', 'michael.foord', 'takluyver', 'serhiy.storchaka', 'yselivanov', 'The Compiler', 'pstch', 'Patrick Grafe']
pr_nums = ['1717', '3778']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue25532'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[cjw296]

The following results in an infinite loop:

>> from inspect import unwrap
>> from unittest.mock import call
>> unwrap(call)

This can occur when you use doctest.DocTestSuite to load doc tests from a module where unittest.mock.call has been imported.

[cjw296]

A naive solution is to chat unittest.mock._Call's __getattr__ to be as follows:

def __getattr__(self, attr):
    if attr == '__wrapped__':
        raise AttributeError('__wrapped__')
    if self.name is None:
        return _Call(name=attr, from_kall=False)
    name = '%s.%s' % (self.name, attr)
    return _Call(name=name, parent=self, from_kall=False)

...but I'm not sure that's the right thing to do.

inspect.unwrap is trying to catch this loop, but the implementation fails for cases where f.__wrapped__ is generative, as in the case of Mock and _Call.

I suspect both modules need a fix, but not sure what best to suggest.

[voidspace]

For mock I think your proposed fix is fine. call is particularly magic as merely importing it into modules can cause introspection problems like this (venusian is a third party library that has a similar issue), so working around these problems as they arise is worthwhile.

[cjw296]

Ah yes, I can see how Venusian would get confused.

How about making the check a more generic:

    if attr.startswith('__'):
        raise AttributeError(attr)

?

That said, why does call have a __getattr__? Where is that intended to be used?

[voidspace]

Have you read the docs for call? :-)

call.foo("foo") generates a call object representing a method call to the method foo. So you can do.

    m = Mock()
    m.foo("foo")

    self.assertEqual(m.mock_calls, call.foo("foo"))

(etc)

See also the call.call_list (I believe) method for assertions on chained calls.

[voidspace]

Preventing the use of "__" attributes would limit the usefulness of call with MagicMock and the use of magic methods. That might be an acceptable trade-off, but would break existing tests for people. (i.e. it's a backwards incompatible change.)

[cjw296]

Indeed, I guess Venusian will get confused, but not sure thats solvable as there will be obvious bugs indicating that call shouldn't be imported at module-level.

This does feel like the problem is actually with inspect.unwrap: there's evidence of an attempt to catch infinite loops like this and blow up with a ValueError, it just doesn't appear those checks are good enough. How many levels of unwrapping are reasonable? 1? 5? 100? It feels like the loop detection should be count, not set-of-ids based...

The other optional we be for _Call instances to be generative only in the right context, or only to a certain depth (10? how deep can one set of calls realistically be?)

I suspect both should probably happen... Thoughts?

[voidspace]

I'm happy to blacklist __wrapped__ in call. I don't see how call can detect that kind of introspection in the general case though. Limiting call depth would be one option, but any limit is going to be arbitrary and I don't "like" that as a solution.

[cjw296]

Cool, what needs to happen for __wrapped__ in to be blacklisted in call?

Separately, inspect.unwrap probably needs to use something less fragile than a set of ids to track whether it's stuck in a loop. What is the actual usecase for __wrapped__ how deeply nested can those realistically expected to be?

[The-Compiler]

FWIW, this also affects pytest users: pytest-dev/pytest#1217

[pstch]

This patch blacklists __wrapped__ (using the same form as the first comment, with a more explicit exception message) in unittest.mock._Call.__getattr__.

I also documented the change and added a tests that checks assertFalse(hasattr(call, '__wrapped__')).

I did not make the same change in the Mock class, as its instances are not usually set at module level (which is what triggers this bug in doctests, as they run inspect.unwrap on module attributes).

I'd like to note that this regression can be nasty for some CI systems : it makes the Python interpreter infinitely allocate memory (as it's not a recursion error) and crashes any host that doesn't limit virtual memory allocation.

[ncoghlan]

An alternative approach would be to change inspect.unwrap() to use getattr_static() rather than the usual getattr().

The downside of that would be potentially breaking true object proxies, like the 3rd party wrapt module and weakref.proxy.

However, what if inspect.signature implemented a limit on the number of recursive descents it permitted (e.g. based on sys.getrecursionlimit()), and then halted the descent, the same way it does if it finds a __signature__ attribute on a wrapped object?

inspect.unwraps makes that relatively straightforward:

    unwrap_count = 0
    recursion_limit = sys.getrecursionlimit()
    def stop_unwrapping(wrapped):
        nonlocal unwrap_count
        unwrap_count += 1
        return unwrap_count >= recursion_limit
    innermost_function = inspect.unwrap(outer_function, stop=stop_unwrapping)

Alternatively, that hard limit on the recursive descent could be baked directly into the loop detection logic in inspect.unwrap (raising ValueError rather than returning the innermost function reached), as Chris suggested above. We'd then just need to check inspect.signature was handling that potential failure mode correctly.

[pstch]

You are right, the fix would be better suited in unwrap.

But, still, shouldn't any __getattr__ implementation take care of not returning, for the __wrapped__ attribute, a dynamic wrapper that provides the same attribute ? __wrapped__ is commonly resolved to the innermost value without __wrapped__, which in this case never happens.

This would also avoid problems with introspection tools that resolve __wrapped__ without the help of unwrap (before Python 3.4 IIRC).