Skip to content
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.

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

test regression dnspython with python 3.5 only: (assert isinstance(ans, greendns.dns.resolver.Answer)) #267

Closed
anthraxx opened this issue Nov 25, 2015 · 7 comments

Comments

@anthraxx
Copy link
Contributor

Hey, me again, sorry for all the noise, I hope creating new issues for different problems is in your interest? Thanks for all your fast feedback.
With an up to date dnspython (1.12.0) two greendns_test.py tests are failing (this happens only with python3.5, python2.7 is working fine):

======================================================================
FAIL: test_query_ans_types (tests.greendns_test.TestHostsResolver)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/python-eventlet/src/eventlet-0.17.4/tests/__init__.py", line 92, in wrapped
    return func(*a, **kw)
  File "/build/python-eventlet/src/eventlet-0.17.4/tests/greendns_test.py", line 116, in test_query_ans_types
    assert isinstance(ans, greendns.dns.resolver.Answer)
AssertionError

======================================================================
FAIL: test_query_unknown_no_raise (tests.greendns_test.TestHostsResolver)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/python-eventlet/src/eventlet-0.17.4/tests/__init__.py", line 92, in wrapped
    return func(*a, **kw)
  File "/build/python-eventlet/src/eventlet-0.17.4/tests/greendns_test.py", line 151, in test_query_unknown_no_raise
    assert isinstance(ans, greendns.dns.resolver.Answer)
AssertionError

Is this a regression in greendns_test within eventlet or may this be a dnspython issue?

@anthraxx anthraxx changed the title test regression dnspython: (assert isinstance(ans, greendns.dns.resolver.Answer)) test regression dnspython with python 3.5 only: (assert isinstance(ans, greendns.dns.resolver.Answer)) Nov 25, 2015
@jstasiak
Copy link
Contributor

Hey @anthraxx, thanks for the report, different issues/tickets for different things is perfectly fine.

This is a funny issue. I managed to nail it down to:

1 There's an import cycle that's wrapped by a try/except block silencing import errors:

# socket.py
greendns = None
if os.environ.get("EVENTLET_NO_GREENDNS", '').lower() != "yes":
    try:
        from eventlet.support import greendns
    except ImportError as ex:
        pass

If instead of silencing the error we print a stack trace we get something like:

Traceback (most recent call last):
  File "/ve/projects/eventlet/eventlet/__init__.py", line 10, in <module>
    from eventlet import convenience
  File "/ve/projects/eventlet/eventlet/convenience.py", line 6, in <module>
    from eventlet.green import socket
  File "/ve/projects/eventlet/eventlet/green/socket.py", line 14, in <module>
    from eventlet.support import greendns
  File "/ve/projects/eventlet/eventlet/support/greendns.py", line 67, in <module>
    setattr(dns.rdtypes, pkg.split('.')[-1], patcher.import_patched(pkg))
  File "/ve/projects/eventlet/eventlet/patcher.py", line 119, in import_patched
    *additional_modules + tuple(kw_additional_modules.items()))
  File "/ve/projects/eventlet/eventlet/patcher.py", line 75, in inject
    _green_thread_modules() +
  File "/ve/projects/eventlet/eventlet/patcher.py", line 348, in _green_socket_modules
    from eventlet.green import socket

2 The import cycle prevents eventlet.support.greendns from being imported by eventlet.green.socket and the only other place in the code that imports it is:

# tests/greendns_test.py
import tests
# ...
try:
    # ...
    import dns.resolver
    # ...
    from eventlet.support import greendns
# ...

3 A behaviour change in Python 3.5 makes that socket -> greendns -> socket import work despite the cycle: https://bugs.python.org/issue17636. The result of this is:

Python < 3.5:

  • import tests imports eventlet.green.socket indirectly, that tries to import eventlet.support.greendns but it fails because of an import cycle
  • import dns.resolver works fine
  • from eventlet.support import greendns is the first and only greendns import that works

Python >= 3.5:

  • import tests imports eventlet.green.socket indirectly, that imports eventlet.support.greendns successfully
  • import dns.resolver works "fine" (I'll get to this)
  • from eventlet.support import greendns is the second import of greendns in the process

4 The point above seems to expose a bug in our patching system. If dns.resolver is imported before greendns all is well. If it's the other way around greendns.dns.resolver is changed silently, therefore invalidating some sane assumptions (like: greendns.HostsAnswer inherits from greendns.dns.resolver.Answer therefore <instance of greendns.HostsAnswer needs to be an instance of greendns.dns.resolver.Answer). Illustration:

# minitest.py
def report():
    print('xx id(greendns.dns.resolver.Answer): %s' % (id(greendns.dns.resolver.Answer),))
    print('xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: %s' % (
        issubclass(greendns.HostsAnswer, greendns.dns.resolver.Answer),
    ))
    print('xx')


import os

if os.environ.get('DNS_FIRST') == 'yes':
    print('xx importing dns.resolver')
    import dns.resolver

print('xx importing greendns')
from eventlet.support import greendns
report()

print('xx importing dns.resolver')
import dns.resolver
report()

Python 2.7:

% DNS_FIRST=yes python minitest.py |grep xx 
xx importing dns.resolver
xx importing greendns
xx id(greendns.dns.resolver.Answer): 140321058273248
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: True
xx
xx importing dns.resolver
xx id(greendns.dns.resolver.Answer): 140321058273248
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: True
xx

% DNS_FIRST=no python minitest.py |grep xx
xx importing greendns
xx id(greendns.dns.resolver.Answer): 140400623429712
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: True
xx
xx importing dns.resolver
xx id(greendns.dns.resolver.Answer): 140400623528752
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: False
xx

Python 3.6 (custom build):

% DNS_FIRST=yes python minitest.py |grep xx                                   
xx importing dns.resolver
xx importing greendns
xx id(greendns.dns.resolver.Answer): 140364823024088
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: True
xx
xx importing dns.resolver
xx id(greendns.dns.resolver.Answer): 140364823024088
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: True
xx

% DNS_FIRST=no python minitest.py |grep xx
xx importing greendns
xx id(greendns.dns.resolver.Answer): 140270206813720
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: True
xx
xx importing dns.resolver
xx id(greendns.dns.resolver.Answer): 140270208303048
xx greendns.HostsAnswer is a subclass of greendns.dns.resolver.Answer: False
xx

I don't immediately understand why this happens and what needs to be done to improve it but at least we know something.

PS. Another thing is, I'm tempted to remove this block from eventlet.green.socket (because the import just didn't work on Python pre 3.5) and, if we want to have green gethostbyname etc. do it right, without import cycles (pinging @temoto here):

greendns = None
if os.environ.get("EVENTLET_NO_GREENDNS", '').lower() != "yes":
    try:
        from eventlet.support import greendns
    except ImportError as ex:
        pass

if greendns:
    gethostbyname = greendns.gethostbyname
    getaddrinfo = greendns.getaddrinfo
    gethostbyname_ex = greendns.gethostbyname_ex
    getnameinfo = greendns.getnameinfo
    __patched__ = __patched__ + ['gethostbyname_ex', 'getnameinfo']

@jstasiak
Copy link
Contributor

@temoto Any ideas regarding the above? I think we're hitting a similar issue when using httplib2 while testing on Python 3.5

@temoto
Copy link
Member

temoto commented Mar 30, 2016

I think it's easy to check if the module was already imported via sys.modules. That would be a workaround crutch. It's entirely clear to me how critical this issue is. Does the issue manifest besides testing?

@jstasiak
Copy link
Contributor

Yes, by testing (above) I mean running a production code in a Python 3 environment, experimentally. I'll try to provide a minimal test case.

@jstasiak
Copy link
Contributor

jstasiak commented Apr 1, 2016

A minimal code to reproduce (latest Eventlet and httplib2, Python 3.5.1):

import httplib2
import eventlet.green.urllib

http = httplib2.Http()
http.request('http://example.org')

The result:

Traceback (most recent call last):
  File "test.py", line 5, in <module>
    print(http.request('http://example.org'))
  File "/Users/me/ve/35/lib/python3.5/site-packages/httplib2/__init__.py", line 1314, in request
    (response, content) = self._request(conn, authority, uri, request_uri, method, body, headers, redirections, cachekey)
  File "/Users/me/ve/35/lib/python3.5/site-packages/httplib2/__init__.py", line 1064, in _request
    (response, content) = self._conn_request(conn, request_uri, method, body, headers)
  File "/Users/me/ve/35/lib/python3.5/site-packages/httplib2/__init__.py", line 1047, in _conn_request
    response = Response(response)
  File "/Users/me/ve/35/lib/python3.5/site-packages/httplib2/__init__.py", line 1382, in __init__
    for key, value in info.items():
AttributeError: 'HTTPResponse' object has no attribute 'items'

The relevant bit of code from httplib2/__init__.py:

        if isinstance(info, http.client.HTTPResponse):  # <- this condition fails although it should not
            for key, value in info.getheaders():
                key = key.lower()
                prev = self.get(key)
                if prev is not None:
                    value = ', '.join((prev, value))
                self[key] = value
            self.status = info.status
            self['status'] = str(self.status)
            self.reason = info.reason
            self.version = info.version
        elif isinstance(info, email.message.Message):
            for key, value in list(info.items()):
                self[key.lower()] = value
            self.status = int(self['status'])
        else:
            for key, value in info.items():  # <- the first condition doesn't match so this is executed and fails
                self[key.lower()] = value
            self.status = int(self.get('status', self.status))

I think that's because existing, already imported http.client module is indirectly replaced when eventlet.green.urllib is imported and this breaks things that already saw the original http.client module, see this:

import http.client
print(['before green import', id(http.client)])
import eventlet.green.urllib
print(['after green import', id(http.client)])

The result:

['before green import', 4491451592]
['after green import', 4500059704]

@temoto
Copy link
Member

temoto commented Apr 1, 2016

@jstasiak thank you very much for minimal case.

I can't right now, will look into this issue later. Maybe it's possible to fix this issue in patcher.

@jstasiak
Copy link
Contributor

I'll extract my test case into a separate issue as I've found another one I think.

jstasiak pushed a commit that referenced this issue May 18, 2016
The green socket module seemed to have only blocking DNS resolution
methods even with dnspython installed which is inconsistent with the
documentation.

This patch has a few consequences:

* an import cycle is eliminated
* if an import cycle reappears here it'll be visible

Note: eliminating the import cycle revealed an issue related to monkey
patching and the way we perform greendns tests (the test failures were
already present on Python 3.5[1] as that version has some import cycle
handling changes). The failures look like this:

======================================================================
FAIL: test_query_ans_types (tests.greendns_test.TestHostsResolver)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kuba/projects/eventlet/tests/greendns_test.py", line 97, in test_query_ans_types
    assert isinstance(ans, greendns.dns.resolver.Answer)
AssertionError

======================================================================
FAIL: test_query_unknown_no_raise (tests.greendns_test.TestHostsResolver)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kuba/projects/eventlet/tests/greendns_test.py", line 129, in test_query_unknown_no_raise
    assert isinstance(ans, greendns.dns.resolver.Answer)
AssertionError

This issue will be addressed in a separate commit.

This patch is contributed by Smarkets Limited.

[1] #267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants