diff --git a/git/util.py b/git/util.py index a694f4fe9..874b764d5 100644 --- a/git/util.py +++ b/git/util.py @@ -1270,8 +1270,8 @@ class BlockingLockFile(LockFile): specified timeout. :note: - If the directory containing the lock was removed, an exception will be raised - during the blocking period, preventing hangs as the lock can never be obtained. + Only an existing lock causes retries. Other errors, including a removed or + unwritable parent directory, are raised immediately. """ __slots__ = ("_check_interval", "_max_block_time") @@ -1297,7 +1297,7 @@ def __init__( def _obtain_lock(self) -> None: """This method blocks until it obtained the lock, or raises :exc:`IOError` if it - ran out of time or if the parent directory was not available anymore. + ran out of time or lock creation failed for a reason other than an existing lock. If this method returns, you are guaranteed to own the lock. """ @@ -1307,10 +1307,14 @@ def _obtain_lock(self) -> None: try: super()._obtain_lock() except OSError as e: - # synity check: if the directory leading to the lockfile is not - # readable anymore, raise an exception + # LockFile retains the creation error as the cause of its OSError. + # Only an existing lock is contention; other errors must not be retried. + if not isinstance(e.__cause__, FileExistsError): + raise + + # Abort if the directory containing the existing lock has disappeared. curtime = time.time() - if not osp.isdir(osp.dirname(self._lock_file_path())): + if not osp.isdir(osp.dirname(self._lock_file_path()) or osp.curdir): msg = "Directory containing the lockfile %r was not readable anymore after waiting %g seconds" % ( self._lock_file_path(), curtime - starttime, diff --git a/test/test_config.py b/test/test_config.py index 8d526457b..e3c30421c 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -3,6 +3,7 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ +import errno import glob import io import os @@ -17,7 +18,7 @@ from git import GitConfigParser, Repo from git.compat import defenc from git.config import _OMD, cp -from git.util import cwd, rmfile +from git.util import BlockingLockFile, cwd, rmfile from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory _tc_lock_fpaths = osp.join(osp.dirname(__file__), "fixtures/*.lock") @@ -28,6 +29,23 @@ def _rm_lock_files(): rmfile(lfp) +@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows") +@pytest.mark.parametrize("error_number", [errno.EACCES, errno.ENOENT]) +def test_blocking_config_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number): + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(GitConfigParser, "t_lock", BlockingLockFile) + error = OSError(error_number, os.strerror(error_number), "config.lock") + with mock.patch.object(os, "open", side_effect=error) as open_lock: + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + GitConfigParser("config", read_only=False) + sleep.assert_not_called() + open_lock.assert_called_once() + assert exc_info.value.__cause__ is error + assert str(exc_info.value) == str(error) + assert not (tmp_path / "config.lock").exists() + + class TestBase(TestCase): def setUp(self): _rm_lock_files() diff --git a/test/test_util.py b/test/test_util.py index d759343f0..ca592aaf7 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -4,7 +4,7 @@ # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ import ast -from datetime import datetime +import errno import os import pathlib import pickle @@ -14,6 +14,7 @@ import tempfile import threading import time +from datetime import datetime from unittest import SkipTest, mock import ddt @@ -37,12 +38,11 @@ LockFile, cygpath, decygpath, - is_cygwin_git, get_user_id, + is_cygwin_git, remove_password_if_present, rmtree, ) - from test.lib import TestBase, requires_symlinks, with_rw_repo @@ -882,3 +882,102 @@ def test_iterable_list_name_lookup_ignores_list_attributes(name, prefix): assert name not in values with pytest.raises(IndexError, match="No item found with id"): values[name] + + +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +@pytest.mark.parametrize("relative", [True, False]) +def test_blocking_lock_waits_for_existing_holder(tmp_path, monkeypatch, path_type, relative): + monkeypatch.chdir(tmp_path) + path = path_type("resource" if relative else tmp_path / "resource") + owner = LockFile(path) + waiter = BlockingLockFile(path, check_interval_s=0.01, max_block_time_s=1) + owner._obtain_lock() + try: + with mock.patch.object(time, "sleep", side_effect=lambda _: owner._release_lock()) as sleep: + waiter._obtain_lock() + sleep.assert_called_once_with(0.01) + assert waiter._has_lock() + finally: + owner._release_lock() + waiter._release_lock() + + +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_reports_timeout(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + owner = LockFile(path_type("resource")) + waiter = BlockingLockFile(path_type("resource"), max_block_time_s=0) + owner._obtain_lock() + try: + with pytest.raises(OSError, match="Waited .* seconds for lock"): + waiter._obtain_lock() + finally: + owner._release_lock() + + +def test_blocking_lock_still_rejects_missing_directory(tmp_path): + waiter = BlockingLockFile(tmp_path / "missing" / "resource") + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, FileNotFoundError) + assert not waiter._has_lock() + + +@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows") +@pytest.mark.parametrize( + "error_number", [errno.EACCES, errno.ENOENT, errno.ENOTDIR, errno.EROFS, errno.ENOSPC, errno.EIO] +) +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +@pytest.mark.parametrize("relative", [True, False]) +def test_blocking_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number, path_type, relative): + monkeypatch.chdir(tmp_path) + path = path_type("resource" if relative else tmp_path / "resource") + waiter = BlockingLockFile(path) + error = OSError(error_number, os.strerror(error_number), waiter._lock_file_path()) + with mock.patch.object(os, "open", side_effect=error) as open_lock: + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + open_lock.assert_called_once() + assert exc_info.value.__cause__ is error + assert str(exc_info.value) == str(error) + assert not waiter._has_lock() + assert not os.path.lexists(waiter._lock_file_path()) + + +@pytest.mark.skipif(os.name != "posix", reason="Directory write permissions require POSIX") +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_rejects_unwritable_cwd(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + original_mode = tmp_path.stat().st_mode + tmp_path.chmod(stat.S_IRUSR | stat.S_IXUSR) + try: + if os.access(".", os.W_OK): + pytest.skip("Current user can write to read-only directories") + assert os.path.isdir(".") + waiter = BlockingLockFile(path_type("resource")) + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, PermissionError) + assert not waiter._has_lock() + finally: + tmp_path.chmod(original_mode) + + +@pytest.mark.skipif(os.name != "posix", reason="Removing the current directory requires POSIX") +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_rejects_removed_cwd(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + tmp_path.rmdir() + waiter = BlockingLockFile(path_type("resource")) + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, FileNotFoundError) + assert not waiter._has_lock()