radarhere
commented
3 weeks ago My thoughts.
More specifically, various registries (like these ones) are kept as global variables. Changing these in multiple threads leads to data races.
I wouldn't expect users to be changing them in multiple threads. I would imagine that for almost all scenarios, users would register their plugins and associated metadata, and then move on. We have Image.register_* functions to add settings. We don't have Image.unregister_* functions to remove settings. If someone found a reason to deprecate the public forms of these registries (ID, OPEN, EXTENSION, etc.) and encourage users to just use our functions instead (register_open(), register_extension(), etc.), that would sound acceptable to me.
Leave them alone and document that registering plugins in multiple threads is not supported (for now?).
I think that modifying plugins is not supported across multiple threads. If multiple threads run
Image.register_open(TiffImageFile.format, TiffImageFile, _accept)at once, that shouldn't pose a problem.
I can imagine that a user might want to load a truncated image and return to normal operations afterwards
from PIL import ImageFile
ImageFile.LOAD_TRUNCATED_IMAGES = True
im = Image.open(...)
im.load()
ImageFile.LOAD_TRUNCATED_IMAGES = Falseand that might lead to unexpected results in a multithreaded environment. Perhaps we need to re-explore passing additional parameters to Image.open(), #569, as a way for users to operate in that scenario.
Similarly to plugins, I don't expect users to be registering multiple StubHandlers. The purpose of test_file_bufrstub's test_handler() is to assert that a StubHandler can be registered and is used by basic image operations. I don't think it needs to test that you can register multiple handlers in separate threads and have those handlers operate in isolation.
I don't expect there is a universal method to ask pytest to run a single test in a single-threaded way - you're running pytest-run-parallel, #8454 is running pytest-freethreaded - so perhaps the simplest solution to the Tests/test_file_bufrstub.py problem is to just return early if the test is already in progress on another thread. I've created #8501 as a suggestion.
lysnikolaou
Apart from the failure in #8454 and other hard crashes at the C level (I'll open separate issues for those), quite a few tests fail when run in multiple threads because of thread-unsafe Python code.
The failures come mainly from three different thread safety issues. Two of those issues come from issues in the tests themselves:
pytest.warnswhich is inherently thread-unsafe. The solution for this is to always run those tests in a single thread.uuid.uuid4()and prepending that to the filenameImage.saveis called with).The third, however, is because of thread-unsafe Python code in Pillow. More specifically, various registries (like these ones) are kept as global variables. Changing these in multiple threads leads to data races. Fixing these is going to be harder, but the possible solutions are:
ContextVars orthreading.locals. This is a backwards-incompatible change though and users that touch these in their code will have to update it to use the context variable APIs (.get()&.set()).Does anyone else have ideas regarding other ways to handle this?
An easy way to reproduce this (though this fails because of this global variable) is to install
pytest-run-paralleland then run the following under the free-threaded build:Failure info
```python rootdir: /Users/lysnikolaou/repos/python/Pillow configfile: pyproject.toml plugins: run-parallel-0.1.0 collected 5 items Tests/test_file_bufrstub.py::test_open PASSED [ 20%] Tests/test_file_bufrstub.py::test_invalid_file PASSED [ 40%] Tests/test_file_bufrstub.py::test_load PASSED [ 60%] Tests/test_file_bufrstub.py::test_save PASSED [ 80%] Tests/test_file_bufrstub.py::test_handler FAILED [100%] ================================================================================= FAILURES ================================================================================== _______________________________________________________________________________ test_handler ________________________________________________________________________________ tmp_path = PosixPath('/private/var/folders/qv/js92y9x526sdmsmjdrylwkx80000gn/T/pytest-of-lysnikolaou/pytest-50/test_handler0') def test_handler(tmp_path: Path) -> None: class TestHandler(ImageFile.StubHandler): opened = False loaded = False saved = False def open(self, im: ImageFile.StubImageFile) -> None: self.opened = True def load(self, im: ImageFile.StubImageFile) -> Image.Image: self.loaded = True im.fp.close() return Image.new("RGB", (1, 1)) def is_loaded(self) -> bool: return self.loaded def save(self, im: Image.Image, fp: IO[bytes], filename: str) -> None: self.saved = True handler = TestHandler() BufrStubImagePlugin.register_handler(handler) with Image.open(TEST_FILE) as im: > assert handler.opened E assert False E + where False =I've verified that the following patch fixes the issue.