patcher diverts subprocess exceptions instead of keeping class identity #413
Comments
temoto
changed the title
|
Thank you, the problem is clear. |
|
This revealed the ugly side of monkey patching, when original stdlib modules are saved for I'm not sure where things start to get wrong, don't have a good idea how to fix this right now. Stay tuned. |
…lass identity reset new reimport module in sys.modules. eventlet#413
|
@temoto see pull/414, inject will return new reimport module. so the exception classes will take the same. |
|
CalledProcessError has been set in globals(), and he is from the new import subprocess module, so when after inject. sys.modules.subprocess keep older, that's why we can't using subprocess.CalledProcessError(sys.modules) to except the error. |
|
Yeah I understand what's going on, so far the best solution I can do is to rebind certain names from stdlib to green module. Please, try this patch eac49d8
|
|
@temoto i have modify like your patch before, but it's not resolved. we can't do that like CalledProcessError = subprocess_orig.CalledProcessError both or that will change CalledProcessError in eventlet.green.subprocess. so we can't define CalledProcessError in eventlet.green.subprocess |
|
@temoto CalledProcessError = sys.modules['subprocess'].CalledProcessError can compatible with subprocess.CalledProcessError and eventlet.green.subprocess.CalledProcessError. |
|
@Linbing I don't understand, can you show what's the problem with code example? |
|
Never mind, I just had to try again with your original example code. Please, try again with this patch.
|
|
@temoto my code example 1、when set eventlet.green.subprocess output is: 2、when set eventlet.green.subprocess the output is the same with before But when we change the example code with delete except eventlet.green.subprocess like 1、using subprocess_orig.CalledProcessError output is 2、 using sys.modules['subprocess'].CalledProcessError output is the real problem is that when we reimport subprocess module, no matter inject or original, the globals CalledProcessError (eventlet.green.CalledProcessError) is new, that's raise CalledProcessError from, so, it's never be instance of sys.modules['subprocess'].CalledProcessError. the only resolution is take globals CalledProcessError (eventlet.green.CalledProcessError) from sys.modules['subprocess'].CalledProcessError, this make globals CalledProcessError equal with sys.modules['subprocess'].CalledProcessError. that's why we can except sys.modules['subprocess'].CalledProcessError or eventlet.green.subprocess.CalledProcessError , because they the some class in memory. You can see my another code example to see class id in memory modify /usr/lib64/python2.7/subprocess.py check_outpu, before raise CalledProcessError. |
|
@temoto that is what i am suggest，but this is only for CalledProcessError resolution，and it's not only the problem of CalledProcessError. all Class used in patched method may casue the same problem. |
|
Yes it's exactly your patch, I'll add authorship. Please test whole package though. And yes it matters for all names. I don't know a good solution without making eventlet contaminate everything with patched modules. |
|
If you encounter same problem for other classes, please write me. |
|
@temoto We're running into the same issue with SubprocessError in openstack/oslo.concurrency [0]. [0] https://bugs.launchpad.net/oslo.concurrency/+bug/1688201 |
|
@YorikSar please try another version
|
|
@temoto Thanks for quick response! Sadly it still doesn't work. You can reproduce the issue like following: Note that with |
|
@YorikSar sorry, but |
|
@temoto But they are still different. I wrote a simple script to check that: % cat check.py
import eventlet
eventlet.monkey_patch()
import subprocess
print(id(subprocess.SubprocessError), id(eventlet.green.subprocess.SubprocessError))
try:
subprocess.check_call('false')
except subprocess.SubprocessError:
print('subprocess.SubprocessError')
except eventlet.green.subprocess.SubprocessError:
print('eventlet.green.subprocess.SubprocessError')
except Exception as ex:
print('Neither', type(ex))
% .tox/py35/bin/python check.py
46457160 46395480
eventlet.green.subprocess.SubprocessErrorAs you can see, |
|
I was checking on Linux, CPython 3.6 Meanwhile, please try to recreate tox environment to exclude cache or something similar. This test must be pretty convincing, right? |
|
@YorikSar also please include this code in your check.py:
|
|
@temoto I did recreate environment that time. Yes, I saw the test and it's strange that it passes although simple check doesn't work. I'll try to run this test again later today with clean venv and different versions of CPython. |
|
@temoto sorry for long waiting. I did this in clean virtualenvs in clean Ubuntu, still no luck, both on CPython 3.5 and 3.6: My guess is that tests run in some dirty context that is different from my |
|
Thanks you for helping. We have special isolated test mechanism that runs separate Python process with a minimal script. I found that monkey_patch/import order different from existing test is causing this problem. So it's testable now, wait for fix. |
|
fwiw this seems like the same as #357 |
|
Yes, it's a duplicate issue, thanks Nat. |
The default Windows hosts file starts something like this:
# Copyright (c) 1993-2009 Microsoft Corp.
#
# This is a sample HOSTS file used by Microsoft TCP/IP for Windows.
...
But `greendns.HostsResolver.LINES_RE` doesn't admit the possibility of a
comment starting at the beginning of a line. It produces a sequence consisting
of data from the comment lines as well as (eventually) the real content of the
hosts file.
This has been ignored since `is_ipv4_addr()` and `is_ipv6_addr()` catch
`dns.exception.SyntaxError` and return `False`. But in a runtime environment
that encounters eventlet#413, `dns.exception.SyntaxError` is not caught, and the
import fails.
Changing '+' to '*' allows `HostsResolver._readlines()` to recognize and skip
comment lines. However, that produces empty strings in the sequence. Filter
out the empty strings by passing the sequence through
`itertools.ifilter(None)`.
In a frozen environment, the runtime bootstrap loader may not properly handle
absolute imports of (e.g.) `dns.exception` when there is no top-level `dns`
package. Relative imports work, e.g.:
from . import exception
But you can't say:
import ..dns.exception
You can't even say:
from .. import dns
because that doesn't work when `dns` is found via `sys.path`.
Since the referencing code consistently uses fully qualified names such as
`dns.exception.SyntaxError`, we could write:
from . import exception as dns_exception
and change every reference to (e.g.) `dns_exception.SyntaxError`.
It was deemed less intrusive to introduce a dummy class as a namespace, e.g.:
class dns(object):
from . import exception
which allows references of the form `dns.exception.SyntaxError` to work as is.
Using relative imports means that `eventlet.support.greendns` need not tinker
with `sys.path`.
Also encapsulate some of the redundancy in `greendns.py`'s `import_patched()`
stanza in a transient `patch_imports()` helper function. Use `try`/`finally`
to ensure `patch_imports()` and the local `import_patched()` are transient.
Also attempt to circumvent eventlet#413 by catching (e.g.)
`eventlet.support.dns.exception.SyntaxError`. It's not clear that this
addresses the problem, however.
The default Windows hosts file starts something like this:
# Copyright (c) 1993-2009 Microsoft Corp.
#
# This is a sample HOSTS file used by Microsoft TCP/IP for Windows.
...
But `greendns.HostsResolver.LINES_RE` doesn't admit the possibility of a
comment starting at the beginning of a line. It produces a sequence consisting
of data from the comment lines as well as (eventually) the real content of the
hosts file.
This has been ignored since `is_ipv4_addr()` and `is_ipv6_addr()` catch
`dns.exception.SyntaxError` and return `False`. But in a runtime environment
that encounters eventlet#413, `dns.exception.SyntaxError` is not caught, and the
import fails.
Changing '+' to '*' allows `HostsResolver._readlines()` to recognize and skip
comment lines. However, that produces empty strings in the sequence. Filter
out the empty strings by passing the sequence through
`itertools.ifilter(None)`.
`greendns.HostsResolver.LINES_RE` doesn't admit the possibility of a comment starting at the beginning of a line. This has been ignored since `is_ipv4_addr()` and `is_ipv6_addr()` catch `dns.exception.SyntaxError` and return `False`. But in a runtime environment that encounters #413, `dns.exception.SyntaxError` is not caught, and the import fails. Changing '+' to '*' allows `HostsResolver._readlines()` to recognize and skip comment lines. greendns.HostsResolver._readlines(), a purely internal method, now returns an itertools generator rather than a list. Change relevant asserts to build a list before comparing.
The python kubernetes client requires a newer version of eventlet. However, newer versions of eventlet have an issue with the subprocess module, which requires subprocess to be imported from eventlet.green instead of being imported directly. See: eventlet/eventlet#413 The eventlet has been upversioned to 0.24.1, therefore, the subprocess import is changed. This update also fixes the issue that raise e triggers 'CalledProcessError' object is not callable error. Change-Id: If3fd8506ececf062ee1b390dc8a87771cb01dec9 Story: 2006980 Task: 37715 Signed-off-by: Tao Liu <tao.liu@windriver.com>
when using eventlet.monkey_patch(), it's will call patch module raise Error change, linke subprocess.check_output or check_call, when somtthing error happened, it's will raise CalledProcessError(retcode, cmd), and this CalledProcessError maybe change in eventlet.green.subprocess, so, we can not except this error like subprocess.CalledProcessError, instead of, we must except this error eventlet.green.subprocess.CalledProcessError.
example:
The text was updated successfully, but these errors were encountered: