fix: prevent deletion of Local values from messing with other ctxs #523
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bellini666
commented
Jul 7, 2025
There was a problem hiding this comment.
note: this is literally a reversion of 85d2445#r161477713, but I added .copy on lines 27 and 32
bellini666
commented
Jul 7, 2025
| assert not hasattr(test_local, "value") | ||
|
|
||
| await asyncio.create_task(_test()) | ||
| assert test_local.value == 123 |
There was a problem hiding this comment.
Without the reversion, and even without the .copy(), this would break, essentially reproducing the issue
carltongibson
approved these changes
Jul 8, 2025
There was a problem hiding this comment.
Looks good @bellini666.
Essentially the suggestion we had in #477. (Always go for the simplest option 🫠)
I'll just wait to see if we get a response on the issue but... 👍 Thanks!
|
OK, user confirming the fix on #485 (comment) — I'll roll a release shortly. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
85d2445#r161477713 changed the
ContextVarto be a dict ofContextVarsWhile that does work in preventing the contexts to leak, removing a value with
del local.valuecompletely removes theContextVar, literally unsetting it from all other contexts.As we discussed here, using
.copy()is simpler and should have no downsides compared to the current solution, especially since it is a shallow copy onlyThis PR reverts the mentioned change and instead uses a
.copy()for both__setattr__and__delattr__. The added tests there still pass for this solution, and I also added a new test that would have caught this issue.Fixes #522
Fixes #485