Running the tests in multiple threads fails because of thread-unsafe Python code

[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:

1. Using pytest.warns which is inherently thread-unsafe. The solution for this is to always run those tests in a single thread.
2. Saving to the same file from multiple threads. The filesystem thus becomes global state, which introduces data races. The solution to this is to make sure different threads write to different files when testing things (for example by using uuid.uuid4() and prepending that to the filename Image.save is 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:

1. Make all of theses ContextVars or threading.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()).
2. Leave them alone and document that registering plugins in multiple threads is not supported (for now?).

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-parallel and then run the following under the free-threaded build:

pytest -v --parallel-threads=100 ./Tests/test_file_bufrstub.py

Failure info

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 = <Tests.test_file_bufrstub.test_handler.<locals>.TestHandler object at 0x200580d01a0>.opened

Tests/test_file_bufrstub.py:76: AssertionError
========================================================================== short test summary info ==========================================================================
FAILED Tests/test_file_bufrstub.py::test_handler - assert False
======================================================================== 1 failed, 4 passed in 0.95s ========================================================================

I've verified that the following patch fixes the issue.

diff --git a/Tests/test_file_bufrstub.py b/Tests/test_file_bufrstub.py
index 77ee5b0ea..0859eb27f 100644
--- a/Tests/test_file_bufrstub.py
+++ b/Tests/test_file_bufrstub.py
@@ -83,4 +83,4 @@ def test_handler(tmp_path: Path) -> None:
         im.save(temp_file)
         assert handler.saved
 
-    BufrStubImagePlugin._handler = None
+    BufrStubImagePlugin._handler.set(None)
diff --git a/src/PIL/BufrStubImagePlugin.py b/src/PIL/BufrStubImagePlugin.py
index 0ee2f653b..49c242246 100644
--- a/src/PIL/BufrStubImagePlugin.py
+++ b/src/PIL/BufrStubImagePlugin.py
@@ -10,11 +10,12 @@
 #
 from __future__ import annotations
 
+from contextvars import ContextVar
 from typing import IO
 
 from . import Image, ImageFile
 
-_handler = None
+_handler = ContextVar('_handler', default=None)
 
 
 def register_handler(handler: ImageFile.StubHandler | None) -> None:
@@ -23,8 +24,7 @@ def register_handler(handler: ImageFile.StubHandler | None) -> None:
 
     :param handler: Handler object.
     """
-    global _handler
-    _handler = handler
+    _handler.set(handler)
 
 
 # --------------------------------------------------------------------
@@ -57,14 +57,15 @@ class BufrStubImageFile(ImageFile.StubImageFile):
             loader.open(self)
 
     def _load(self) -> ImageFile.StubHandler | None:
-        return _handler
+        return _handler.get()
 
 
 def _save(im: Image.Image, fp: IO[bytes], filename: str | bytes) -> None:
-    if _handler is None or not hasattr(_handler, "save"):
+    handler = _handler.get()
+    if handler is None or not hasattr(handler, "save"):
         msg = "BUFR save handler not installed"
         raise OSError(msg)
-    _handler.save(im, fp, filename)
+    handler.save(im, fp, filename)
 
 
 # --------------------------------------------------------------------

[radarhere]

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 = False

and 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]

Perhaps we need to re-explore passing additional parameters to Image.open(), #569, as a way for users to operate in that scenario.

This is a good idea, I'll explore this. Making these ContextVars (or introducing new ones that people should use if they want to be changing them from multiple threads) sounds more straightforward though.