[issue1514] module behaviour incompatibility which makes eventlet fail
New submission from YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>: the following code behaves differently on cpython and pypy. it breaks eventlet monkey pathcing. --------------- import select import sys f = select.select setattr(select, 'select', None) assert f != select.select # save, re-import, restore. # this is essentially what eventlet.patcher does. modname = 'select' saved = sys.modules.get(modname) sys.modules.pop(modname) __import__(modname) sys.modules[modname] = saved assert f != select.select # this succeeds on cpython but fails on pypy. ---------- messages: 5843 nosy: pypy-issue, yamt priority: bug status: unread title: module behaviour incompatibility which makes eventlet fail ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: Uh. I won't ask why eventlet does it (as on CPython it seems to have no effect). Instead we can fix it so that re-importing built-in modules has no effect either on PyPy. ---------- nosy: +arigo status: unread -> chatting ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Amaury Forgeot d Arc <amauryfa@gmail.com> added the comment: Noteworthy fact: "import select" will reload the module, AND store it in the current namespace __import__('select') will reload the module BUT leave the old module in the namespace. CPython and PyPy both initialize a builtin module only once, and make a shallow copy of its initial __dict__. On the next import, this initial dict is used to refresh the module, no init function is called. The difference is that CPython will create a new module instance each time, when PyPy has only one prebuilt instance (stored in space.builtin_modules). This won't be a simple change. As a workaround, instead of saving the module, you could save a copy of its __dict__ instead: saved = sys.modules.get(modname).__dict__.copy() ...pop, import... sys.modules[modname].__dict__.update(saved) ---------- nosy: +amaury ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: I thought I fixed it in f26956c61773; at least, enough to have the reported test case work. But indeed, I didn't fix the difference reported by Amaury: repeating an import on a built-in or extension module which has been removed from sys.modules will, on CPython, create a new module object. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: Indeed, the fix I did is wrong: it's possible to write a simpler test that now fails (but this simpler test as not written so far, bad us). Reverted my changes to the code for now. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: amaury's workaround to update __dict__ would work for this simple testcase. but unfortunately it doesn't make much sense for eventlet because it wants to use both of patched and unpatched versions of the module. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: probably this is a better test case. ----------------- import sys import select original = select.select m = __import__('select') setattr(m, 'select', None) assert select.select == None assert m.select == None # reload del sys.modules['select'] import select assert select.select == original assert m.select == None # this fails on pypy ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Amaury Forgeot d Arc <amauryfa@gmail.com> added the comment: "use both of patched and unpatched versions of the module." Did you try to copy the module then? m = type(sys)('select') m.__dict__.update(select.__dict__) Of course pypy should do it in __import__. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: unfortunately it's difficult (well, at least for me) to workaround by copying modules in the case of eventlet because the library and application both freely does import. ie. there's no single entity to control import. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: how about this patch? ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: ping! ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: Can this logic be put somewhere in pypy/module/imp/importing.py? Or at least be activated only in that case, which is "force_init=True"? I'm hesitant to change the default behavior of getbuiltinmodule(). ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: assuming that force_init=True is used only by load_module, i think activating it only for force_init=True makes sense. or probably add another argument to specify the behavior, rather than abusing force_init. do you want me to prepare a patch? ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Brian Kearns <bdkearns@gmail.com> added the comment: conditional on force_init sounds reasonable. please make sure the patch enables and passes the two test_builtin_reimport tests in pypy/module/imp/test/test_app.py. ---------- nosy: +bdk ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Brian Kearns <bdkearns@gmail.com> added the comment: fixed in 0ecfb7242213 ---------- status: chatting -> resolved ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Brian Kearns <bdkearns@gmail.com> added the comment: (turns out that broke things, but it is a place to start for a patch) ---------- status: resolved -> chatting ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: here's a new version of patch. (pypy-import.diff2) "./pypy-c ./pytest.py -A pypy/module/imp" passed. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Brian Kearns <bdkearns@gmail.com> added the comment: simplified/applied in 6002c93c0cc0, thanks. ---------- status: chatting -> resolved ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Brian Kearns <bdkearns@gmail.com> added the comment: had to back out this patch -- failures in pypy/module/zipimport tests also would like the fix to include changes to imp.init_builtin to match cpython ---------- status: resolved -> chatting ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: "./pypy-c ./pytest.py -A -v pypy/module/zipimport" passed for me. ("43 passed") can you explain? ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Brian Kearns <bdkearns@gmail.com> added the comment: ./pytest.py pypy/module/zipimport ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: bdk: I cannot reproduce this. test_zipimport_deflated seems to pass for me since 4e0378f68161, which is just after 6002c93c0cc0. The backout in c402f888f629 doesn't seem to have fixed (or broken) anything because no test from there seemed to fail at that time. Can you double-check? ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Brian Kearns <bdkearns@gmail.com> added the comment: with yamt patch -- zipimport tests fail. at some point i tried modified/simplified patches but never got something that both translated and passed all tests, so everything was backed out to previous state. at some point may have had something that translated and passed zipimport, but failed elsewhere. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: the problem seems that "unlazy" part of my patch ends up to loading "struct" module's app level code, which imports "sturct" module again and causes recursions. we should add sys.modules before initializing the module. new version of a patch, pypy-import.diff3, passes at least zipimport tests. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: ping! ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: If the test "isinstance(w_mod, Module)" fails then we used to add anyway w_mod to sys.modules. With your patch we no longer do. Is that intentional? ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: no, it isn't intentional. i don't know when it can be false. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: A lot of things can subtly go wrong with this patch, because it creates a new module as an instance of the base class "Module", which can lead to all sorts of unexpected issues... For instance, if you do "del sys.modules['sys']; import sys" then you get a sys module that is not really a sys module, e.g. it would never have the attribute 'exc_type'... Unsure how to solve this. ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: i wasn't aware how exc_type is implemented in pypy. it seems incompatible with cpython. shouldn't it be fixed regardless of this issue? ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: Sorry if you don't get much progress on this issue. To reiterate, the problem is that it's delicate. The way the built-in modules are currently implemented in PyPy creates some friction with CPython. In order to really fix it we need more than a small localized change. Now, it's possible that there is a small patch that just fixes the issue for eventlet. Such a change would still be fine if we can't think of other cases that it breaks, but in this case, pypy-import.diff3 does break other things... ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: the symptom seems same as cpython. kuma% python Python 2.7.6 (default, Feb 11 2014, 04:39:17) [GCC 4.5.3] on netbsd6 Type "help", "copyright", "credits" or "license" for more information.
import sys del sys.modules['sys'] import sys print sys.exc_type Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'module' object has no attribute 'exc_type'
________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: Ok, I'll just accept this patch if it shows no issue with the test suite from CPython (started test run in the new branch 'issue1514'). Did you check it with eventlet too? ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment: i tried some of ryu's unit tests, which uses eventlet. git clone https://github.com/osrg/ryu.git cd ryu pypy ./ryu/tests/run_tests.py unit.lib.test_hub 31fa36a1d761 (including the patch) -> hanged up f2bdd3baf2d1 (not including the patch) -> passed ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> added the comment:
31fa36a1d761 (including the patch) -> hanged up f2bdd3baf2d1 (not including the patch) -> passed
oops. correctly, 31fa36a1d761 (not including the patch) -> hanged up f2bdd3baf2d1 (including the patch) -> passed ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
Armin Rigo <armin.rigo@gmail.com> added the comment: Ok, marked as resolved (f96656bbecc9). ---------- status: chatting -> resolved ________________________________________ PyPy bug tracker <tracker@bugs.pypy.org> <https://bugs.pypy.org/issue1514> ________________________________________
participants (4)
-
Amaury Forgeot d Arc -
Armin Rigo
-
Brian Kearns
-
YAMAMOTO Takashi