Repository: django/asgiref

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:
carltongibson: Thanks @spanezz 👍 

The type hints need adjusting to keep mypy happy. (See CI) 

I had a glance at the different commits, but will try and reason them through this week. 
carltongibson: @spanezz You can run the type checker locally by doing (e.g.) `tox -e py311-mypy`. 

OK, everything passes with 16d4ae269b81a3bf956e43791819cacd6ce8df0a Option 1. Can I ask you to pull 7b3ce47dc2631e70b1d173d02d6f8801b1e9f0af Option 2 into a separate PR (after this one, I guess). 

I think we should go with Option 1 now, and release a v3.8.2 to fix the issue, and then we can consider whether Option 2 is worth adopting for a v3.9 at a more leisurely pace. //cc @andrewgodwin 
spanezz: > @spanezz You can run the type checker locally by doing (e.g.) `tox -e py311-mypy`.

Thank you for the pointer! I am uncomfortable running code randomly downloaded off the internet on my devel machine, and I'm unfortunately not equipped with a suitably containerized alternative.

> OK, everything passes with [16d4ae2](https://github.com/django/asgiref/commit/16d4ae269b81a3bf956e43791819cacd6ce8df0a) Option 1. Can I ask you to pull [7b3ce47](https://github.com/django/asgiref/commit/7b3ce47dc2631e70b1d173d02d6f8801b1e9f0af) Option 2 into a separate PR (after this one, I guess).
> 
> I think we should go with Option 1 now, and release a v3.8.2 to fix the issue, and then we can consider whether Option 2 is worth adopting for a v3.9 at a more leisurely pace. //cc @andrewgodwin
> 
> I closed #477 which had the `self._data.set({**self._data.get({}), key: value})` option.

Done (I removed the simplification commit from this PR and opened #481)
andrewgodwin: This definitely seems like a pretty clean change and easier to understand than the bigger rewrite, so let's get this in.

I removed the python 3.8 tests from main - if you want to rebase on top of that so we get a clean sweep, I'm happy to merge this.
spanezz: > This definitely seems like a pretty clean change and easier to understand than the bigger rewrite, so let's get this in.
> 
> I removed the python 3.8 tests from main - if you want to rebase on top of that so we get a clean sweep, I'm happy to merge this.

Thanks, and done!

Patch:
diff --git a/asgiref/local.py b/asgiref/local.py
index a8b9459..7d228ae 100644
--- a/asgiref/local.py
+++ b/asgiref/local.py
@@ -2,37 +2,38 @@ import asyncio
 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 d50cba2..cdcbd28 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 @@ def test_thread_critical_local_not_context_dependent_in_sync_thread():
     # 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