From e7b61a49e1a972a02da783748f608a21f8934c4e Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Tue, 4 Apr 2023 17:24:47 -0700 Subject: [PATCH 1/8] Surprisingly simple fix to not working properly with Python threads --- src/filelock/_windows.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/filelock/_windows.py b/src/filelock/_windows.py index cb324c9..d21ec70 100644 --- a/src/filelock/_windows.py +++ b/src/filelock/_windows.py @@ -3,6 +3,7 @@ import os import sys from errno import ENOENT +from threading import local from typing import cast from ._api import BaseFileLock @@ -11,7 +12,7 @@ if sys.platform == "win32": # pragma: win32 cover import msvcrt - class WindowsFileLock(BaseFileLock): + class WindowsFileLock(BaseFileLock, local): """Uses the :func:`msvcrt.locking` function to hard lock the lock file on windows systems.""" def _acquire(self) -> None: From 199c3618521ce3a2fdd0fb9102be07c51fc83d7c Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Tue, 4 Apr 2023 17:42:25 -0700 Subject: [PATCH 2/8] Move lower to fix issue on all platforms --- src/filelock/_api.py | 4 ++-- src/filelock/_windows.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/filelock/_api.py b/src/filelock/_api.py index 50f1c3d..855797d 100644 --- a/src/filelock/_api.py +++ b/src/filelock/_api.py @@ -6,7 +6,7 @@ import time import warnings from abc import ABC, abstractmethod -from threading import Lock +from threading import local, Lock from types import TracebackType from typing import Any @@ -36,7 +36,7 @@ def __exit__( self.lock.release() -class BaseFileLock(ABC, contextlib.ContextDecorator): +class BaseFileLock(ABC, contextlib.ContextDecorator, local): """Abstract base class for a file lock object.""" def __init__( diff --git a/src/filelock/_windows.py b/src/filelock/_windows.py index d21ec70..cb324c9 100644 --- a/src/filelock/_windows.py +++ b/src/filelock/_windows.py @@ -3,7 +3,6 @@ import os import sys from errno import ENOENT -from threading import local from typing import cast from ._api import BaseFileLock @@ -12,7 +11,7 @@ if sys.platform == "win32": # pragma: win32 cover import msvcrt - class WindowsFileLock(BaseFileLock, local): + class WindowsFileLock(BaseFileLock): """Uses the :func:`msvcrt.locking` function to hard lock the lock file on windows systems.""" def _acquire(self) -> None: From b8d671ff465a46d45ab9a8766c841da5fe0c8768 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 5 Apr 2023 00:42:37 +0000 Subject: [PATCH 3/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/filelock/_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/filelock/_api.py b/src/filelock/_api.py index 855797d..2158ac2 100644 --- a/src/filelock/_api.py +++ b/src/filelock/_api.py @@ -6,7 +6,7 @@ import time import warnings from abc import ABC, abstractmethod -from threading import local, Lock +from threading import Lock, local from types import TracebackType from typing import Any From 1f704f08c0cfa84baef98da0318cf20e56c0e825 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Tue, 4 Apr 2023 22:04:51 -0700 Subject: [PATCH 4/8] Add some threadpool-thrash tests --- tests/test_filelock.py | 74 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/tests/test_filelock.py b/tests/test_filelock.py index 406a62d..1aac639 100644 --- a/tests/test_filelock.py +++ b/tests/test_filelock.py @@ -5,6 +5,7 @@ import os import sys import threading +from concurrent.futures import ThreadPoolExecutor from contextlib import contextmanager from errno import ENOSYS from inspect import getframeinfo, stack @@ -12,6 +13,7 @@ from stat import S_IWGRP, S_IWOTH, S_IWUSR, filemode from types import TracebackType from typing import Callable, Iterator, Tuple, Type, Union +from uuid import uuid4 import pytest from _pytest.logging import LogCaptureFixture @@ -509,3 +511,75 @@ def test_soft_errors(tmp_path: Path, mocker: MockerFixture) -> None: mocker.patch("os.open", side_effect=OSError(ENOSYS, "mock error")) with pytest.raises(OSError, match="mock error"): SoftFileLock(tmp_path / "a.lock").acquire() + + +@pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) +def test_thrashing_with_threadpool_passing_lock_to_threads(tmp_path: Path, lock_type: BaseFileLock) -> None: + lock_file = tmp_path / "test.txt.lock" + txt_file = tmp_path / "test.txt" + + # notice how lock is passed to the function below + lock = lock_type(lock_file) + + def mess_with_file(lock, txt_file): + with lock: + for _ in range (3): + u = str(uuid4()) + txt_file.write_text(u) + assert txt_file.read_text() == u + + return True + + results = [] + with ThreadPoolExecutor() as executor: + for _ in range(100): + results.append(executor.submit(mess_with_file, lock, txt_file)) + + assert all([r.result() for r in results]) + + +@pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) +def test_thrashing_with_threadpool_global_lock(tmp_path: Path, lock_type: BaseFileLock) -> None: + lock_file = tmp_path / "test.txt.lock" + txt_file = tmp_path / "test.txt" + + # Notice how lock is scoped to be allowed in the nested function below + lock = lock_type(lock_file) + + def mess_with_file(txt_file): + with lock: + for _ in range (3): + u = str(uuid4()) + txt_file.write_text(u) + assert txt_file.read_text() == u + + return True + + results = [] + with ThreadPoolExecutor() as executor: + for _ in range(100): + results.append(executor.submit(mess_with_file, txt_file)) + + assert all([r.result() for r in results]) + + +@pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) +def test_thrashing_with_threadpool_lock_recreated_in_each_thread(tmp_path: Path, lock_type: BaseFileLock) -> None: + lock_file = tmp_path / "test.txt.lock" + txt_file = tmp_path / "test.txt" + + def mess_with_file(lock_type, lock_file, txt_file): + with lock_type(lock_file): + for _ in range (3): + u = str(uuid4()) + txt_file.write_text(u) + assert txt_file.read_text() == u + + return True + + results = [] + with ThreadPoolExecutor() as executor: + for _ in range(100): + results.append(executor.submit(mess_with_file, lock_type, lock_file, txt_file)) + + assert all([r.result() for r in results]) From d0b41cfb4b32d806fbaa95c5f1ca2071848a855c Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Wed, 5 Apr 2023 11:44:30 -0700 Subject: [PATCH 5/8] Fix CI --- tests/test_filelock.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_filelock.py b/tests/test_filelock.py index 1aac639..000daea 100644 --- a/tests/test_filelock.py +++ b/tests/test_filelock.py @@ -514,16 +514,16 @@ def test_soft_errors(tmp_path: Path, mocker: MockerFixture) -> None: @pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) -def test_thrashing_with_threadpool_passing_lock_to_threads(tmp_path: Path, lock_type: BaseFileLock) -> None: +def test_thrashing_with_thread_pool_passing_lock_to_threads(tmp_path: Path, lock_type: BaseFileLock) -> None: lock_file = tmp_path / "test.txt.lock" txt_file = tmp_path / "test.txt" # notice how lock is passed to the function below lock = lock_type(lock_file) - def mess_with_file(lock, txt_file): + def mess_with_file(lock: BaseFileLock, txt_file: Path): with lock: - for _ in range (3): + for _ in range(3): u = str(uuid4()) txt_file.write_text(u) assert txt_file.read_text() == u @@ -539,16 +539,16 @@ def mess_with_file(lock, txt_file): @pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) -def test_thrashing_with_threadpool_global_lock(tmp_path: Path, lock_type: BaseFileLock) -> None: +def test_thrashing_with_thread_pool_global_lock(tmp_path: Path, lock_type: BaseFileLock) -> None: lock_file = tmp_path / "test.txt.lock" txt_file = tmp_path / "test.txt" # Notice how lock is scoped to be allowed in the nested function below lock = lock_type(lock_file) - def mess_with_file(txt_file): + def mess_with_file(txt_file: Path): with lock: - for _ in range (3): + for _ in range(3): u = str(uuid4()) txt_file.write_text(u) assert txt_file.read_text() == u @@ -564,13 +564,13 @@ def mess_with_file(txt_file): @pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) -def test_thrashing_with_threadpool_lock_recreated_in_each_thread(tmp_path: Path, lock_type: BaseFileLock) -> None: +def test_thrashing_with_thread_pool_lock_recreated_in_each_thread(tmp_path: Path, lock_type: BaseFileLock) -> None: lock_file = tmp_path / "test.txt.lock" txt_file = tmp_path / "test.txt" - def mess_with_file(lock_type, lock_file, txt_file): + def mess_with_file(lock_type: type, lock_file: Path, txt_file: Path): with lock_type(lock_file): - for _ in range (3): + for _ in range(3): u = str(uuid4()) txt_file.write_text(u) assert txt_file.read_text() == u From 1b62a85b6be1c458d145b73dace03ada808c32b9 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Wed, 5 Apr 2023 11:47:37 -0700 Subject: [PATCH 6/8] Remove _thread_lock as now the file lock itself will be thread-local --- src/filelock/_api.py | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/filelock/_api.py b/src/filelock/_api.py index 2158ac2..958369a 100644 --- a/src/filelock/_api.py +++ b/src/filelock/_api.py @@ -6,7 +6,7 @@ import time import warnings from abc import ABC, abstractmethod -from threading import Lock, local +from threading import local from types import TracebackType from typing import Any @@ -67,9 +67,6 @@ def __init__( # The mode for the lock files self._mode: int = mode - # We use this lock primarily for the lock counter. - self._thread_lock: Lock = Lock() - # The lock counter is used for implementing the nested locking mechanism. Whenever the lock is acquired, the # counter is increased and the lock is only released, when this value is 0 again. self._lock_counter: int = 0 @@ -168,18 +165,16 @@ def acquire( poll_interval = poll_intervall # Increment the number right at the beginning. We can still undo it, if something fails. - with self._thread_lock: - self._lock_counter += 1 + self._lock_counter += 1 lock_id = id(self) lock_filename = self._lock_file start_time = time.perf_counter() try: while True: - with self._thread_lock: - if not self.is_locked: - _LOGGER.debug("Attempting to acquire lock %s on %s", lock_id, lock_filename) - self._acquire() + if not self.is_locked: + _LOGGER.debug("Attempting to acquire lock %s on %s", lock_id, lock_filename) + self._acquire() if self.is_locked: _LOGGER.debug("Lock %s acquired on %s", lock_id, lock_filename) break @@ -194,8 +189,7 @@ def acquire( _LOGGER.debug(msg, lock_id, lock_filename, poll_interval) time.sleep(poll_interval) except BaseException: # Something did go wrong, so decrement the counter. - with self._thread_lock: - self._lock_counter = max(0, self._lock_counter - 1) + self._lock_counter = max(0, self._lock_counter - 1) raise return AcquireReturnProxy(lock=self) @@ -206,17 +200,16 @@ def release(self, force: bool = False) -> None: :param force: If true, the lock counter is ignored and the lock is released in every case/ """ - with self._thread_lock: - if self.is_locked: - self._lock_counter -= 1 + if self.is_locked: + self._lock_counter -= 1 - if self._lock_counter == 0 or force: - lock_id, lock_filename = id(self), self._lock_file + if self._lock_counter == 0 or force: + lock_id, lock_filename = id(self), self._lock_file - _LOGGER.debug("Attempting to release lock %s on %s", lock_id, lock_filename) - self._release() - self._lock_counter = 0 - _LOGGER.debug("Lock %s released on %s", lock_id, lock_filename) + _LOGGER.debug("Attempting to release lock %s on %s", lock_id, lock_filename) + self._release() + self._lock_counter = 0 + _LOGGER.debug("Lock %s released on %s", lock_id, lock_filename) def __enter__(self) -> BaseFileLock: """ From b29fbb8247d21091f89bc4488a0c2ad59f0fffd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bern=C3=A1t=20G=C3=A1bor?= Date: Wed, 5 Apr 2023 11:55:51 -0700 Subject: [PATCH 7/8] PR feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Bernát Gábor --- tests/test_filelock.py | 75 +++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 45 deletions(-) diff --git a/tests/test_filelock.py b/tests/test_filelock.py index 000daea..1f27af1 100644 --- a/tests/test_filelock.py +++ b/tests/test_filelock.py @@ -513,73 +513,58 @@ def test_soft_errors(tmp_path: Path, mocker: MockerFixture) -> None: SoftFileLock(tmp_path / "a.lock").acquire() -@pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) -def test_thrashing_with_thread_pool_passing_lock_to_threads(tmp_path: Path, lock_type: BaseFileLock) -> None: - lock_file = tmp_path / "test.txt.lock" - txt_file = tmp_path / "test.txt" - - # notice how lock is passed to the function below - lock = lock_type(lock_file) +def _check_file_read_write(txt_file: Path) -> None: + for _ in range(3): + uuid = str(uuid4()) + txt_file.write_text(uuid) + assert txt_file.read_text() == uuid - def mess_with_file(lock: BaseFileLock, txt_file: Path): - with lock: - for _ in range(3): - u = str(uuid4()) - txt_file.write_text(u) - assert txt_file.read_text() == u - return True +@pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) +def test_thrashing_with_thread_pool_passing_lock_to_threads(tmp_path: Path, lock_type: type[BaseFileLock]) -> None: + def mess_with_file(lock_: BaseFileLock) -> None: + with lock_: + _check_file_read_write(txt_file) + lock_file, txt_file = tmp_path / "test.txt.lock", tmp_path / "test.txt" + lock = lock_type(lock_file) results = [] with ThreadPoolExecutor() as executor: for _ in range(100): - results.append(executor.submit(mess_with_file, lock, txt_file)) + results.append(executor.submit(mess_with_file, lock)) - assert all([r.result() for r in results]) + assert all(r.result() is None for r in results) @pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) -def test_thrashing_with_thread_pool_global_lock(tmp_path: Path, lock_type: BaseFileLock) -> None: - lock_file = tmp_path / "test.txt.lock" - txt_file = tmp_path / "test.txt" - - # Notice how lock is scoped to be allowed in the nested function below - lock = lock_type(lock_file) - - def mess_with_file(txt_file: Path): +def test_thrashing_with_thread_pool_global_lock(tmp_path: Path, lock_type: type[BaseFileLock]) -> None: + def mess_with_file() -> None: with lock: - for _ in range(3): - u = str(uuid4()) - txt_file.write_text(u) - assert txt_file.read_text() == u - - return True + _check_file_read_write(txt_file) + lock_file, txt_file = tmp_path / "test.txt.lock", tmp_path / "test.txt" + lock = lock_type(lock_file) results = [] with ThreadPoolExecutor() as executor: for _ in range(100): - results.append(executor.submit(mess_with_file, txt_file)) + results.append(executor.submit(mess_with_file)) - assert all([r.result() for r in results]) + assert all(r.result() is None for r in results) @pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) -def test_thrashing_with_thread_pool_lock_recreated_in_each_thread(tmp_path: Path, lock_type: BaseFileLock) -> None: - lock_file = tmp_path / "test.txt.lock" - txt_file = tmp_path / "test.txt" - - def mess_with_file(lock_type: type, lock_file: Path, txt_file: Path): +def test_thrashing_with_thread_pool_lock_recreated_in_each_thread( + tmp_path: Path, + lock_type: type[BaseFileLock], +) -> None: + def mess_with_file() -> None: with lock_type(lock_file): - for _ in range(3): - u = str(uuid4()) - txt_file.write_text(u) - assert txt_file.read_text() == u - - return True + _check_file_read_write(txt_file) + lock_file, txt_file = tmp_path / "test.txt.lock", tmp_path / "test.txt" results = [] with ThreadPoolExecutor() as executor: for _ in range(100): - results.append(executor.submit(mess_with_file, lock_type, lock_file, txt_file)) + results.append(executor.submit(mess_with_file)) - assert all([r.result() for r in results]) + assert all(r.result() is None for r in results) From a2ccce38f9545abb2795fe3a14ad108281b5d6f7 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Wed, 5 Apr 2023 21:18:47 -0700 Subject: [PATCH 8/8] Add some more skips for if tests are run in an environment that has python run as root --- tests/test_filelock.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_filelock.py b/tests/test_filelock.py index 1f27af1..f261b7c 100644 --- a/tests/test_filelock.py +++ b/tests/test_filelock.py @@ -83,6 +83,10 @@ def tmp_path_ro(tmp_path: Path) -> Iterator[Path]: @pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) @pytest.mark.skipif(sys.platform == "win32", reason="Windows does not have read only folders") +@pytest.mark.skipif( + sys.platform != "win32" and os.geteuid() == 0, # noqa: SC200 + reason="Cannot make a read only file (that the current user: root can't read)", +) def test_ro_folder(lock_type: type[BaseFileLock], tmp_path_ro: Path) -> None: lock = lock_type(str(tmp_path_ro / "a")) with pytest.raises(PermissionError, match="Permission denied"): @@ -98,6 +102,10 @@ def tmp_file_ro(tmp_path: Path) -> Iterator[Path]: @pytest.mark.parametrize("lock_type", [FileLock, SoftFileLock]) +@pytest.mark.skipif( + sys.platform != "win32" and os.geteuid() == 0, # noqa: SC200 + reason="Cannot make a read only file (that the current user: root can't read)", +) def test_ro_file(lock_type: type[BaseFileLock], tmp_file_ro: Path) -> None: lock = lock_type(str(tmp_file_ro)) with pytest.raises(PermissionError, match="Permission denied"):