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

Simplify local's implementation #481

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

spanezz
Copy link
Contributor

@spanezz spanezz commented Oct 9, 2024

Note: this PR applies over #478.

This is a proposal for a simplification of asgiref.local.Local as a thin adapter layer over either threading.local or ContextVar.

@andrewgodwin
Copy link
Member

I get the idea of this, and I think it makes sense, but this is deep in the part of the codebase that we can land a big change in, all be confident that it works great, and then it mysteriously breaks Django right afterwards.

Are there any downsides in your mind to redoing it like this?

@spanezz
Copy link
Contributor Author

spanezz commented Oct 10, 2024

I get the idea of this, and I think it makes sense, but this is deep in the part of the codebase that we can land a big change in, all be confident that it works great, and then it mysteriously breaks Django right afterwards.

Are there any downsides in your mind to redoing it like this?

The only downside that I can think of is that I might be missing something important, and my approach is wrong. I tried to follow the effect of the code also on Django's main branch, and I still couldn't see downsides.

Especially when some infrastructure is deep and core, I feel motivated to try and make it as minimal and clearly defined as possible. I find it tends to pay off in the medium-long term, as each extra code or layer makes things harder to reason and debug, and it tends to be more critical the more core a piece of code is. When studying the code for #478 I spotted this opportunity and tried to explore it.

I'd say that the code simplification, if it turns out to be valid, would ripple in the definition of thread_critical as an argument to Local: if thread_critical=True, the contents of the Local are shared between asyncio tasks, and if thread_critical=False asyncio tasks inherit the Local values but the changes they perform on it are isolated.

The asgiref test suite would agree with this, and thread_critical is not used by other asgiref code. Django uses it only in django.db.utils.ConnectionHandler to, as I understand it, prevent reusing the same database handle between different threads, while cross-talk between different asyncio tasks is ok. Indeed if a Task opens a DB connection, it's ok if other tasks reuse it.

If I read that correctly, then Django's ConnectionHandler could be refactored to use a threading.local, reducing the abstraction load needed to follow that critical code. asgiref.local.Local could then eventually loose thread_critical, and focus on being a threading.local extension that also isolates asyncio Tasks.

I would understand if this is a can of worms you don't want to open, though to me they look like yummy worms :)

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.

2 participants