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

Reimplement request attribute access w/ __getattr__ #5617

Merged
merged 3 commits into from Nov 22, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Nov 22, 2017

Supersedes #5576. Many thanks to @yotamofek for the original PR.

@carltongibson
Copy link
Collaborator

carltongibson commented Nov 22, 2017

@rpkilby OK, too early... Tests pass but if __getattribute__ raises the AttributeError, why is __getattr__ not then (re-)called?

@carltongibson carltongibson added this to the v3.7.4 milestone Nov 22, 2017
@rpkilby
Copy link
Member Author

rpkilby commented Nov 22, 2017

I might have this completely wrong, as I don't know much about the cpython internals, but... my understanding is that the underlying c code calls __getattribute__ and if an AttributeError is raised, it then calls __getattr__ (if defined). The default implementation of __getattribute__ isn't directly calling __getattr__, which is why we don't see an infinite loop.

In essence:

  • Underlying C code calls Request.__getattribute__ which fails and raises an AttributeError.
  • Underlying C code calls Request.__getattr__, which checks the WSGIRequest.
  • WSGIRequest check fails, so Request.__getattr__ calls Request.__getattribute__ to raise the exception.

@rpkilby
Copy link
Member Author

rpkilby commented Nov 22, 2017

I just did a simple timeit test, and the PR does seem to be faster (which makes sense, since there are less caught exceptions. I used the following test:

class Perf(TestCase):
    def test_perf(self):
        from timeit import repeat

        wsgi_request = factory.get('/')
        request = Request(wsgi_request)

        # non-existent attribute
        def foo():
            try:
                request.foo
            except AttributeError:
                pass

        # attribute exists on WSGIRequest
        def body():
            request.body

        # attribute exists on DRF Request
        def _request():
            request._request

        print(min(repeat(foo, number=100000)))
        print(min(repeat(body, number=100000)))
        print(min(repeat(_request, number=100000)))

        raise Exception('done')
Timings foo body _request
before 0.57 s 0.36 s 0.07 s
after 0.25 s 0.13 s 0.01 s

@carltongibson
Copy link
Collaborator

carltongibson commented Nov 22, 2017

OK. It does read that way. Good linking skills.

@carltongibson carltongibson merged commit 1a667f4 into encode:master Nov 22, 2017
1 check passed
@rpkilby rpkilby deleted the request-attribute-access branch Nov 22, 2017
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Add tests for proxying WSGIRequest attributes in Request.

* Add request attribute exception test

* Reimplement request attribute access
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 this pull request may close these issues.

None yet

3 participants