python-pillow / python-pillow/Pillow

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

Open
#8,492 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Free-threading
Dominant language
Python
Stars
13.8k
Forks
2.5k
Avg merge
2d 8h
Merged PRs (30d)
89

Description

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)
 
 
 # --------------------------------------------------------------------

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with Tests/test_file_bufrstub.py and src/PIL/BufrStubImagePlugin.py, then reproduce the failure with pytest-run-parallel using the command shown in the issue. Trace the handler registry and the other global registries implicated by parallel tests. Done means the affected tests pass safely when run across multiple threads without introducing incompatible registry behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.