Make use_original thread-local to fix cross-thread races#1318
Open
jfindlay wants to merge 2 commits into
Open
Conversation
The use_original flag on FakeOsModule was a process-global class attribute toggled by the use_original_os() context manager. When one thread entered the context manager while another thread was in the middle of a faked filesystem operation, the faked call could observe use_original=True and dispatch to the real os module, failing against paths that only exist in the fake filesystem. Store use_original in a threading.local and have use_original_os() save and restore the previous thread-local value so nested uses continue to work. The class attribute becomes an instance property, preserving the existing instance.use_original get/set API. A regression test in fake_filesystem_unittest_test exercises the concurrent-worker pattern with explicit hammer threads entering use_original_os() alongside asyncio.to_thread workers; without the fix it fails reliably with PermissionError against the fake root. Fixes pytest-dev#1317.
jfindlay
force-pushed
the
thread-local-use-original
branch
from
5a0202f to
0bb1cff
Compare
for more information, see https://pre-commit.ci
Member
|
"Unit tests passing" is not completely true - the new test is failing in one build. I haven't looked into it yet, may have a closer look tonight... |
Member
|
Ok, I can reproduce the test failure locally under WSL/ubuntu with pypy 3.10, and it looks like this is another threading issue related to the use of |
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
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.
[This bug was found, isolated, fixed, and reported by Claude Opus 4.7, with some minor edits by me. You are welcome to update or adapt as needed.]
Fixes #1317.
Describe the changes
The
use_originalflag onFakeOsModulewas a process-global class attribute toggled by theuse_original_os()context manager. When one thread entered the context manager while another thread was in the middle of a faked filesystem operation, the faked call could observeuse_original=Trueand dispatch to the realosmodule, failing against paths that only exist in the fake filesystem.Store
use_originalin athreading.localand haveuse_original_os()save and restore the previous thread-local value so nested uses continue to work. The class attribute becomes an instance property, preserving the existinginstance.use_originalget/set API.A regression test in
fake_filesystem_unittest_testexercises the concurrent-worker pattern with explicit hammer threads enteringuse_original_os()alongsideasyncio.to_threadworkers; without the fix it fails reliably withPermissionErroragainst the fake root.Tasks