Repository: django/asgiref (language: python)

Issue:
asgiref.local.Local fails to isolate changes between asyncio tasks
`asgiref.local.Local` used to isolate changes performed in asyncio tasks in pre-3.7, and stopped doing so from 3.7+. This is likely an issue, since the point of `asgiref.local.Local` compared to `threading.local` ought to be to provide locals also for asyncio concurrency.

I created a complete reproducer in [asgiref-test.py.gz](https://github.com/user-attachments/files/17235594/asgiref-test.py.gz)

I tracked the issue [to the way asgiref.local.Local sets values](https://github.com/django/asgiref/blob/main/asgiref/local.py#L27): if I read it correctly it does it in a way that it changes the dict in the previous context: it tries to do a get/update/set cycle, but it updates the mutable dict returned by `get()`,  so the side effect propagates to the state reachable before `set` is called.

Indeed, chaging that line with `storage_object = self._data.get({}).copy()` makes it behave how I would expect.


Discussion / hints:
This is causing big issues in Django when is running under ASGI. The language of the user is randomly changed for different requests as the `active` language is stored in `local` variable.  When we run under ASGI `3.8+`, somehow the data for one request is affected by another request...

To reproduce:
- just create a route to the view which returns current language for current user (for instance provided in cookies)
- login as same/another user and switch the language to different one
- if we will execute curl constantly to get the language (which should not be changed as it should be taken from cookie), we will randomly see different languge (exactly at the time when we do any action as another user)
@spanezz Do you mean from 3.8+. 

The change you linked was added then: 

https://github.com/django/asgiref/commit/d920c3c44f59ef037be3a09a41a5640014cfb1e2

Are you able to reproduce in a test case?

Fancy making a PR with your suggestion? 
> @spanezz Do you mean from 3.8+.

Ah right. I read mentions of contextvar in 3.7.0 changelog, and I notice now it was related to a different part.

> Are you able to reproduce in a test case?

Yes: I attached [asgiref-test.py.gz](https://github.com/user-attachments/files/17235594/asgiref-test.py.gz) with unittest code to my issue, and it works stand-alone. I could try integrating it in asgiref's test suite, but I have no experience with pytest.

Also, to do it right I would need a specification about the exact expected behaviour of Local in the cases of both thread and asyncio tasks, and so far I failed to find one.

> Fancy making a PR with your suggestion?

I think my suggestion is a proof of concept but not an actual solution: it would mean that the operation of setting one value in a Local would have a complexity that grows linearly with the number of variables in the local (due to the dict copy involved).

An efficient solution may need a rethinking of the underlying storage, and I guess having my test case above integrated in asgiref's test suite may help with that.
@spanezz great, thanks for confirming. That all makes sense. 

I'll have a look at the test, and have a think but probably need @andrewgodwin to comment on the proposed change. 
Local is meant to keep a local context per asyncio context/coroutine, specifically per-request in the case of Django (of course). It doesn't need to map perfectly via sync_to_async/etc. as long as it works per-request; we used to do this by having a specific thing set on request context entry, but I believe that's gone now (it's been a couple years since I looked at that code).

If you have a suggested PR, please drop it and we'll discuss it, otherwise pin asigref to the lower version and I'll try to find some time to go relearn all this code in the next few months.
Hey @andrewgodwin, I'm reviewing #348 and following from there again. 

> ...we used to do this by having a specific thing set on request context entry...

I can't see this. Can you point to somewhere (doesn't matter when) where that happened? (I can trace it from a starting point.) 

Thanks. 🙏 
As per your comment @spanezz, not updating the storage object in place seems to address the issue: 

```diff
❯ git diff
diff --git a/asgiref/local.py b/asgiref/local.py
index a8b9459..784c570 100644
--- a/asgiref/local.py
+++ b/asgiref/local.py
@@ -24,9 +24,8 @@ class _CVar:
         if key == "_data":
             return super().__setattr__(key, value)
 
-        storage_object = self._data.get({})
-        storage_object[key] = value
-        self._data.set(storage_object)
+        # Update a copy of the existing storage.
+        self._data.set({**self._data.get({}), key: value})
 
     def __delattr__(self, key: str) -> None:
         storage_object = self._data.get({})
```

You say: 

> ...it would mean that the operation of setting one value in a Local would have a complexity that grows linearly with the number of variables in the local (due to the dict copy involved).

I'm not sure though that [copying the dict is going to be particularly slow](https://github.com/python/cpython/blob/16cd6cc86b3ba20074ae3eefb61aeb24ee1544f7/Objects/dictobject.c#L919). 

* Are we expecting a lot of storage in a `Local`? 
* Is there any other correct method? (O(n) beats wrong, especially for small n, in general.)

🤔

I need to look at exactly how this plays out against various Django versions (ref #475) and then I can prepare a PR for comment. 

> * Are we expecting a lot of storage in a `Local`?

I guess it depends a lot on what people are going to do with it: considering it behaves like a global dict, I couldn't blame one who might decide to store a whole parsed config file into it.

> * Is there any other correct method? (O(n) beats wrong, especially for small n, in general.)

I was planning to spend tomorrow afternoon integrating the tests above into asgiref, then experimenting with turning Local into a dict of `ContextVar`s: it whould make Local a very thin adapter over straightforward contextvar usage, and it would simplify Local's implementation significantly.

I prototyped that approach on a different codebase, and so far I can't see a reason why it wouldn't work.

> Local is meant to keep a local context per asyncio context/coroutine, specifically per-request in the case of Django (of course). It doesn't need to map perfectly via sync_to_async/etc. as long as it works per-request; we used to do this by having a specific thing set on request context entry, but I believe that's gone now (it's been a couple years since I looked at that code).

Thanks, that helps a lot!

In the case of asyncio context/coroutines, can I assume that each time an ASGI application is invoked it is run in its own asyncio Task? I tried to find it in ASGI documentation and failed. I verified it is the case with Daphne (which wraps the application invocation with `asyncio.ensure_future`), and I don't see how to make `asgiref.local.Local` isolate changes between different coroutines running in the same Task.

> If you have a suggested PR, please drop it and we'll discuss it, otherwise pin asigref to the lower version and I'll try to find some time to go relearn all this code in the next few months.

Thanks! I plan to work on it tomorrow if nobody beats me to it
@spanezz Sounds good. Look forward to seeing it. 👍 

Fix patch:
diff --git a/asgiref/local.py b/asgiref/local.py
index a8b9459b..7d228aeb 100644
--- a/asgiref/local.py
+++ b/asgiref/local.py
@@ -2,37 +2,38 @@
 import contextlib
 import contextvars
 import threading
-from typing import Any, Dict, Union
+from typing import Any, Union
 
 
 class _CVar:
     """Storage utility for Local."""
 
     def __init__(self) -> None:
-        self._data: "contextvars.ContextVar[Dict[str, Any]]" = contextvars.ContextVar(
-            "asgiref.local"
-        )
+        self._data: dict[str, contextvars.ContextVar[Any]] = {}
 
-    def __getattr__(self, key):
-        storage_object = self._data.get({})
+    def __getattr__(self, key: str) -> Any:
         try:
-            return storage_object[key]
+            var = self._data[key]
         except KeyError:
             raise AttributeError(f"{self!r} object has no attribute {key!r}")
 
+        try:
+            return var.get()
+        except LookupError:
+            raise AttributeError(f"{self!r} object has no attribute {key!r}")
+
     def __setattr__(self, key: str, value: Any) -> None:
         if key == "_data":
             return super().__setattr__(key, value)
 
-        storage_object = self._data.get({})
-        storage_object[key] = value
-        self._data.set(storage_object)
+        var = self._data.get(key)
+        if var is None:
+            self._data[key] = var = contextvars.ContextVar(key)
+        var.set(value)
 
     def __delattr__(self, key: str) -> None:
-        storage_object = self._data.get({})
-        if key in storage_object:
-            del storage_object[key]
-            self._data.set(storage_object)
+        if key in self._data:
+            del self._data[key]
         else:
             raise AttributeError(f"{self!r} object has no attribute {key!r}")
 


Test patch:
diff --git a/tests/test_local.py b/tests/test_local.py
index d50cba21..cdcbd280 100644
--- a/tests/test_local.py
+++ b/tests/test_local.py
@@ -1,6 +1,7 @@
 import asyncio
 import gc
 import threading
+from threading import Thread
 
 import pytest
 
@@ -338,3 +339,39 @@ async def async_function():
     # inner value was set inside a new async context, meaning that
     # we do not see it, as context vars don't propagate up the stack
     assert not hasattr(test_local_not_tc, "test_value")
+
+
+def test_visibility_thread_asgiref() -> None:
+    """Check visibility with subthreads."""
+    test_local = Local()
+    test_local.value = 0
+
+    def _test() -> None:
+        # Local() is cleared when changing thread
+        assert not hasattr(test_local, "value")
+        setattr(test_local, "value", 1)
+        assert test_local.value == 1
+
+    thread = Thread(target=_test)
+    thread.start()
+    thread.join()
+
+    assert test_local.value == 0
+
+
+@pytest.mark.asyncio
+async def test_visibility_task() -> None:
+    """Check visibility with asyncio tasks."""
+    test_local = Local()
+    test_local.value = 0
+
+    async def _test() -> None:
+        # Local is inherited when changing task
+        assert test_local.value == 0
+        test_local.value = 1
+        assert test_local.value == 1
+
+    await asyncio.create_task(_test())
+
+    # Changes should not leak to the caller
+    assert test_local.value == 0