Skip to content

Fix #487 reference cycles in Local #508

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fowczarek
Copy link

@fowczarek fowczarek commented Apr 11, 2025

Fix reference cycles in Local. A more detailed description is available in the issue.
Fix #487

Explanation

When Django tries to reference a database connection that is not yet established, it executes something like this (paraphrasing):

from asgiref.local import Local

connections = Local()

def get_connection(alias: str):
    try:
        return getattr(connections, alias)
    except AttributeError:
        conn = create_new_connection(alias)
        setattr(connections, alias, conn)
        return conn

Now, internally, asgiref's Local does this:

def __getattr__(self, key):
    with self._lock_storage() as storage:
        return getattr(storage, key)

@contextlib.contextmanager
def _lock_storage(self):
    if self._thread_critical:
        try:
            asyncio.get_running_loop()
        except RuntimeError:
            yield self._storage
        else:
            ...
    else:
        ...

Now, putting everything together:

  1. Django calls getattr on the Local object
  2. The _lock_storage context manager is entered
  3. It attempts to find the asyncio.get_running_loop(), which raises a RuntimeError
  4. The exception handler yields self._storage (at this point, we're still in the exception handler inside the context manager)
  5. Local executes getattr on storage, which raises an AttributeError
  6. The AttributeError is propagated back to the context manager and since it's in the exception handler, it's linked to the previous RuntimeError (Python assumes the AttributeError was raised while attempting to handle the RuntimeError)
  7. At this point, both exceptions hold each other referenced (one through exception chaining, the other through the traceback)
  8. They also hold everything up to the point in my code where I attempted to use the database connection referenced, preventing those objects from being freed as well

@carltongibson
Copy link
Member

carltongibson commented Apr 12, 2025

@fowczarek Thanks for this. Just to clarify for posterity, the reason this works is because we exit the exception handler before proceeding with the sync pathway, thereby escaping the reference cycle described in the issue, is that right?

@fowczarek fowczarek force-pushed the gh-487-fix-reference-cycles-in-asgiref.local.Local branch from bc3b28a to d0fdde6 Compare April 15, 2025 08:45
@fowczarek
Copy link
Author

@carltongibson Yes. I copied the explanation part of the issue to increase PR readability.

@fowczarek
Copy link
Author

@carltongibson Should I take any more action on this PR?

@carltongibson
Copy link
Member

@fowczarek — no, it looks good. (It was DjangoCon Europe last week, so everything was on pause; thanks for your patience.)

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.

asgiref.local.Local creates reference cycles that require garbage collection to be freed when executed in a sync context
2 participants