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

Break reference cycle between request and context. #3649

Merged
merged 5 commits into from
Jan 20, 2021

Conversation

luhn
Copy link
Contributor

@luhn luhn commented Jan 15, 2021

When implementing traversal, it's common (at least for me) to store the request in the resource objects so you can do things like database lookups.

class MyResource:
    def __init__(self, request):
        self.request = request

    def __getitem__(self, key):
        if self.request.db.lookup(key):
            return ChildResource(self.request, key)
        else:
            raise KeyError()

I realized this morning that this will cause a reference cycle between request.context and context.request. Not the end of the world, obviously, but unnecessary pressure on the GC.

Maybe this is the user's responsibility? We could document not to store the request in the context, or to use a WeakRef.

The framework can also prevent this by breaking the cycle once the request has been processed, which is what this PR does. Really it's just a single line (request.__dict__.pop('context', None)), but it got a bit more complicated than that because several router tests rely on the context object being in the request.

src/pyramid/router.py Outdated Show resolved Hide resolved
@mmerickel
Copy link
Member

LGTM, do you mind updating the changelog?

@luhn
Copy link
Contributor Author

luhn commented Jan 15, 2021

Done. I'll also backport to 1.10.

luhn added a commit to luhn/pyramid that referenced this pull request Jan 15, 2021
luhn added a commit to luhn/pyramid that referenced this pull request Jan 15, 2021
mmerickel added a commit that referenced this pull request Jan 15, 2021
Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor reST syntax fix.

CHANGES.rst Outdated Show resolved Hide resolved
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@luhn
Copy link
Contributor Author

luhn commented Jan 20, 2021

Anything more that needs to happen on this before merge?

@stevepiercy, I made the requested rst changes.

@mmerickel
Copy link
Member

It's ready once @stevepiercy releases it.

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I must have missed the GitHub notification of a pushed commit.

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

Successfully merging this pull request may close these issues.

3 participants