Skip to content

Use of RecursionError breaks compatibility with Python 2.7 and 3.4 #1619

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

Closed
mschmitzer opened this issue Jun 23, 2021 · 5 comments · Fixed by #1624
Closed

Use of RecursionError breaks compatibility with Python 2.7 and 3.4 #1619

mschmitzer opened this issue Jun 23, 2021 · 5 comments · Fixed by #1624

Comments

@mschmitzer
Copy link

elasticsearch-py version: 7.13.2

Description of the problem including expected versus actual behavior:

In commit cfdfd51, use of the RecursionError class was introduced. This class was added in Python 3.5, though.

This causes errors as this under Python 2.7:

    def perform_request(
        self, method, url, params=None, body=None, timeout=None, ignore=(), headers=None
    ):
        url = self.url_prefix + url
        if params:
            url = "%s?%s" % (url, urlencode(params))
    
        full_url = self.host + url
    
        start = time.time()
        orig_body = body
        try:
            kw = {}
            if timeout:
                kw["timeout"] = timeout
    
            # in python2 we need to make sure the url and method are not
            # unicode. Otherwise the body will be decoded into unicode too and
            # that will fail (#133, #201).
            if not isinstance(url, str):
                url = url.encode("utf-8")
            if not isinstance(method, str):
                method = method.encode("utf-8")
    
            request_headers = self.headers.copy()
            request_headers.update(headers or ())
    
            if self.http_compress and body:
                body = self._gzip_compress(body)
                request_headers["content-encoding"] = "gzip"
    
            response = self.pool.urlopen(
                method, url, body, retries=Retry(False), headers=request_headers, **kw
            )
            duration = time.time() - start
            raw_data = response.data.decode("utf-8", "surrogatepass")
>       except RecursionError:
E       NameError: global name 'RecursionError' is not defined

../../../.venv/py2/lib/python2.7/site-packages/elasticsearch/connection/http_urllib3.py:256: NameError

According to #1295 and setup.py, Python 2.7 and 3.4 should still be supported, though.

@sethmlarson
Copy link
Contributor

sethmlarson commented Jun 23, 2021

Thanks for opening this. I'm kind of surprised this made it through considering we have unit tests and integration tests both using Python 2.7 and 3.4 and it doesn't look like any of them are failing with this, doing a dive into this to see why that's happening.

Until then you can downgrade to 7.13.1 and the next patch release (7.13.3) will contain a fix.

@sethmlarson
Copy link
Contributor

sethmlarson commented Jun 23, 2021

I've yanked the v7.13.2 release due to this issue, until the next patch release is out I recommend using v7.13.1.

@mschmitzer
Copy link
Author

Thanks for looking into this. I guess this hasn't shown up in the tests because the specific error is not triggered. As far as I understand, the except expression is not evaluated unless there actually is an exception to handle.

The error is actually easy to trigger by trying to talk to a non-existent server:

from elasticsearch.client import Elasticsearch
client = Elasticsearch("http://localhost:9", max_retries=0)  # nothing listens on this port
client.indices.stats("_all")

@sethmlarson
Copy link
Contributor

sethmlarson commented Jun 23, 2021

@MarcSchmitzer Yeah, thanks for doing this analysis. The except clause sits right on the boundary of components being tested. We mock and test the happy path for HTTP connections, the error handling component for HTTP connections (but not via a raise which would have triggered the error) and our integration test suite doesn't test errors only APIs.

I'm remedying this gap in our test suite in the fix as well.

@sethmlarson
Copy link
Contributor

Closed in #1609

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

Successfully merging a pull request may close this issue.

2 participants