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

green package (more than one module level) weirdness #316

Open
jstasiak opened this issue May 18, 2016 · 1 comment
Open

green package (more than one module level) weirdness #316

jstasiak opened this issue May 18, 2016 · 1 comment

Comments

@jstasiak
Copy link
Contributor

Extracted from #267 (comment)

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]
@jstasiak
Copy link
Contributor Author

Another example of this (Eventlet fb067b6). See eventlet.support.greendns.HostsAnswer class definition:

# ...
class HostsAnswer(dns.resolver.Answer):
    """Answer class for HostsResolver object"""
# ...

Minimal test case:

# t.py
from __future__ import print_function

from eventlet.support import greendns

import sys
if sys.argv[1] == 'yes':
    import dns.resolver


print(sys.argv[1], issubclass(greendns.HostsAnswer, greendns.dns.resolver.Answer))
# Python 2.7
% python t.py yes
yes False
% python t.py no 
no True

# Python 3.5.1
% python t.py yes
yes False
% python t.py no 
no True

jstasiak pushed a commit that referenced this issue May 18, 2016
The issue can be demonstrated by running the following piece of code:

    # t.py
    from __future__ import print_function

    from eventlet.support import greendns

    import sys
    if sys.argv[1] == 'yes':
        import dns.resolver

    print(sys.argv[1], issubclass(greendns.HostsAnswer, greendns.dns.resolver.Answer))

The results:

    # Python 2.7.11
    % python t.py yes
    yes False
    % python t.py no
    no True

    # Python 3.5.1
    % python t.py yes
    yes False
    % python t.py no
    no True

Interestingly enough this particular test issue was only affecting Python
3.5+ before 861d684. Why?

* This issue appears to be caused by importing green version of a package
  being followed by importing a non-green version of the same package
* When we run tests using nose it first imports the main tests module
  (tests/__init__.py) which imports eventlet, that imports
  eventlet.convenience and that then imports eventlet.green.sockt.
* Before 861d684 on Python < 3.5 the eventlet.green.socket import mentioned
  above would fail to import greendns (because of an import cycle) so when
  running those tests greendns was only being correctly imported *after* the
  regular dns
* Since 861d684 (or on Python 3.5+) the green socket module correctly
  imports greendns which means that when the regular dns subpackages are
  being imported in this test file greendns is already imported and the
  patching issue demonstrated by the code above is in effect

The patching/greening weirdness is reported[1] now.

Fixes #267

This patch is contributed by Smarkets Limited.

[1] #316
jstasiak added a commit that referenced this issue Jun 15, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

[1] getsentry/raven-python#703
[2] #316

This patch is contributed by Smarkets Limited.
jstasiak added a commit that referenced this issue Jun 15, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

Changes to the originl http package code involve:

* Removing unnecessary import(s)
* Replacing some regular imports with eventlet.green imports
* Replacing fullmatch()[3] usage with match() so we stay Python 3.3
  compatible

I left urllib.parse imports intact as nothing there performs IO.

[1] getsentry/raven-python#703
[2] #316
[3] https://docs.python.org/3/library/re.html#re.fullmatch

This patch is contributed by Smarkets Limited.
jstasiak added a commit that referenced this issue Jun 15, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

Changes to the originl http package code involve:

* Removing unnecessary import(s)
* Replacing some regular imports with eventlet.green imports
* Replacing fullmatch()[3] usage with match() so we stay Python 3.3
  compatible

I left urllib.parse imports intact as nothing there performs IO.

Green httplib module is also modified because it used to import
http.client using patcher which was breaking things the same way.

[1] getsentry/raven-python#703
[2] #316
[3] https://docs.python.org/3/library/re.html#re.fullmatch

This patch is contributed by Smarkets Limited.
jstasiak added a commit that referenced this issue Jun 15, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

Changes to the originl http package code involve:

* Removing unnecessary import(s)
* Replacing some regular imports with eventlet.green imports
* Replacing fullmatch()[3] usage with match() so we stay Python 3.3
  compatible

I left urllib.parse imports intact as nothing there performs IO.

Green httplib module is also modified because it used to import
http.client using patcher which was breaking things the same way.

[1] getsentry/raven-python#703
[2] #316
[3] https://docs.python.org/3/library/re.html#re.fullmatch

This patch is contributed by Smarkets Limited.
jstasiak added a commit that referenced this issue Jun 15, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

Changes to the originl http package code involve:

* Removing unnecessary import(s)
* Replacing some regular imports with eventlet.green imports
* Replacing fullmatch()[3] usage with match() so we stay Python 3.3
  compatible

I left urllib.parse imports intact as nothing there performs IO.

Green httplib module is also modified because it used to import
http.client using patcher which was breaking things the same way.

[1] getsentry/raven-python#703
[2] #316
[3] https://docs.python.org/3/library/re.html#re.fullmatch

This patch is contributed by Smarkets Limited.
jstasiak added a commit that referenced this issue Jun 15, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

Changes to the originl http package code involve:

* Removing unnecessary import(s)
* Replacing some regular imports with eventlet.green imports
* Replacing fullmatch()[3] usage with match() so we stay Python 3.3
  compatible

I left urllib.parse imports intact as nothing there performs IO.

Green httplib module is also modified because it used to import
http.client using patcher which was breaking things the same way.

[1] getsentry/raven-python#703
[2] #316
[3] https://docs.python.org/3/library/re.html#re.fullmatch

This patch is contributed by Smarkets Limited.
jstasiak added a commit that referenced this issue Jul 8, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

Changes to the originl http package code involve:

* Removing unnecessary import(s)
* Replacing some regular imports with eventlet.green imports
* Replacing fullmatch()[3] usage with match() so we stay Python 3.3
  compatible

I left urllib.parse imports intact as nothing there performs IO.

Green httplib module is also modified because it used to import
http.client using patcher which was breaking things the same way.

A new dependency, enum-compat, is added to ensure that the enum module
is present on Python 3.3 (the http package code comes the latest Python
development branch and uses enum).

[1] getsentry/raven-python#703
[2] #316
[3] https://docs.python.org/3/library/re.html#re.fullmatch

This patch is contributed by Smarkets Limited.
jstasiak added a commit that referenced this issue Jul 8, 2016
The Eventlet patcher and the way we were patching multi-level http
package don't work well[1][2]. I spent a lot of time trying to make it
work but in the end every solution I came up with was breaking something
else and made the patching and providing green http even more
complicated - I wouldn't envy anyone having to debug it in the future.

After a lot of thinking I decided having our own copy of http with the
necessary modifications applied seems like the most straightforward and
the most reliable solution, even considering its downsides (we need to
keep it up to date ourselves and the API won't 100 % match the regular
http module API on older Python 3 versions as our bundled version is the
most recent one and has bug fixes and extra features implemented).

The code introduces by this commit comes from the following Python
commit (development branch):

commit 6251d66ba9a692d3adf5d2e6818b29ac44130787
Author: Xavier de Gaye <xdegaye@users.sourceforge.net>
Date:   2016-06-15 11:35:29 +0200

    Issue #26862: SYS_getdents64 does not need to be defined on android
    API 21.

Changes to the original http package code involve:

* Removing unnecessary import(s)
* Replacing some regular imports with eventlet.green imports
* Replacing fullmatch()[3] usage with match() so we stay Python 3.3
  compatible

I left urllib.parse imports intact as nothing there performs IO.

Green httplib module is also modified because it used to import
http.client using patcher which was breaking things the same way.

A new dependency, enum-compat, is added to ensure that the enum module
is present on Python 3.3 (the http package code comes the latest Python
development branch and uses enum).

[1] getsentry/raven-python#703
[2] #316
[3] https://docs.python.org/3/library/re.html#re.fullmatch

This patch is contributed by Smarkets Limited.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant