asgiref.local.Local fails to isolate changes between asyncio tasks

[spanezz]

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

I tracked the issue to the way asgiref.local.Local sets values: 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.

[sshishov]

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)

[carltongibson]

@spanezz Do you mean from 3.8+.

The change you linked was added then:

d920c3c

Are you able to reproduce in a test case?

Fancy making a PR with your suggestion?

[spanezz]

@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 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.

[carltongibson]

@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.

[andrewgodwin]

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.

[carltongibson]

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. 🙏

[carltongibson]

As per your comment @spanezz, not updating the storage object in place seems to address the issue:

❯ 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.

• 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.

[spanezz]

• 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 ContextVars: 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.

[spanezz]

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

[carltongibson]

@spanezz Sounds good. Look forward to seeing it. 👍

[spanezz]

@spanezz Sounds good. Look forward to seeing it. 👍

Done in PR #478: the functions I added to the test suite reproduce the issue I reported here, and all tests pass using a dict of contextvars as a backend storage.

[spanezz]

I added an extra commit that simplifies local.py, by leveraging the fact that ContextVar is automatically thread-local.

All tests pass with that, too. If you consider it too disruptive, I'm happy to rework it it as a separate PR.

[andrewgodwin]

I commented in both the PRs - I will say that the simplification one is failing tests all over the place, so a little more work there needed, but the simpler slight change seems good.

[spanezz]

I commented in both the PRs - I will say that the simplification one is failing tests all over the place, so a little more work there needed, but the simpler slight change seems good.

Tests are failing because of mypy, since the PR codes Local as a def instead of a class for compatibility, and executors: "Local" = Local() becomes invalid from mypy's perspective.

Preserving code compatibility while keeping Local usable as a type sounds like a tricky discussion, and I wouldn't get started on that unless there's interest in this POC