Infinite recursion in ast rewriter

[ionelmc]

Weird issue in one of my tests (https://travis-ci.org/ionelmc/python-manhole/jobs/269038610#L361-L361)

tests/test_manhole.py:63: in test_log_when_uninstalled
    import manhole
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/manhole.py:49: in <module>
    _ORIGINAL_FDOPEN = _get_original('os', 'fdopen')
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/manhole.py:39: in _get_original
    return getattr(_original(mod), name)
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/eventlet/patcher.py:194: in original
    real_mod = __import__(modname, {}, {}, modname.split('.')[:-1])
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/_pytest/assertion/rewrite.py:98: in find_module
    if not self._should_rewrite(name, fn_pypath, state):
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/_pytest/assertion/rewrite.py:166: in _should_rewrite
    if fn_pypath.fnmatch(pat):
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/py/_path/common.py:249: in fnmatch
    return FNMatcher(pattern)(self)
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/py/_path/common.py:445: in __call__
    return fnmatch.fnmatch(name, pattern)
.tox/2.7-eventlet-nocov/lib/python2.7/fnmatch.py:40: in fnmatch
    import os
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/_pytest/assertion/rewrite.py:98: in find_module
    if not self._should_rewrite(name, fn_pypath, state):
.tox/2.7-eventlet-nocov/lib/python2.7/site-packages/_pytest/assertion/rewrite.py:166: in _should_rewrite
    if fn_pypath.fnmatch(pat):
E   RuntimeError: maximum recursion depth exceeded while calling a Python object
!!! Recursion detected (same locals & position)

Is this enough to figure out the problem? I'd put my money on that lazy "import os" as the cause.

[RonnyPfannschmidt]

this pretty much looks like eventlet monkeypatchign breaks the world

likely elevated due to pylibs general design misstakes

[nicoddemus]

That line was changed in #2140, and is indeed related to fnmatch not being imported at that moment.

@ionelmc perhaps try to import os and/or fnmatch before importing manhole?

[RonnyPfannschmidt]

@nicoddemus i'm just shocked that we collectively missed that this very call site had an exact warning about the code we switched it to

[ionelmc]

I've tried importing os, fnmatch in various places, it doesn't fix it.

Preimporting doesn't solve it because import foo will always trigger the import hook, regardless of what's in sys.modules. Maybe avoid fnmatch? Can pytest just use fnmatchcase? That one doesn't seem to have any local imports.

So behold, the minimal reproducer:

import sys
sys.modules.pop('os')
import os


def test_foo():
    pass

It appears the problem is eventlet removing stuff from sys.modules. It looks silly but can pytest support it?

[RonnyPfannschmidt]

wait a moment, eventlet removes stuff from sys.modules breaking expectations

when they break the world with magic they should fix it

[RonnyPfannschmidt]

https://github.com/eventlet/eventlet/blob/master/eventlet/patcher.py - just NOPE - we dont clean up after that

[RonnyPfannschmidt]

i believe we should be closing this one as "eventlet is quite the event"

[ionelmc]

I'm fine with that I guess. But how hard is to support it anyway? Morbid curiosity is all 😁

[RonnyPfannschmidt]

i have no idea, they literally just break the world - i suspect it takes about a week of research to fix this consistently and it will be horrid code

[flub]

Oh, I had a giant re-write of the eventlet patcher a few years back to try and make that sane. But somehow stopped caring too much about eventlet before trying to get that finished/merged.

On missing the comment, reading the issue discussions it seems like the comment was read but should have been updated as it should no longer be applicable. I guess one way to fix this to figure out another way of solving #2121, possibly by just implementing fnmatch from scratch or so. I certainly feel something for the "eventlet is quite the event" attitude, just like pytest it's historic evolution has been hard on it though. ;-)

[nicoddemus]

Closing as this has not seen activity in awhile.