From 99815f20363d5feed1894a37f888a94298e299cc Mon Sep 17 00:00:00 2001 From: Ray Osborn Date: Fri, 2 Oct 2026 15:51:40 -0500 Subject: [PATCH] Harden NXLock release and construction - Guard os.close in release() so the lock file is always removed and fd reset - Initialize fd before any exception can be raised in __init__, so __del__ is safe after an invalid lock directory - Return the lock from __enter__ so `with NXLock(f) as lock` binds it - Enforce a minimum 0.1 s polling interval to avoid busy-waiting - Correct the lock_file/fd and is_stale docstrings - Add tests for the constructor failure and context manager; clean up locks left behind by the removal-error tests Co-Authored-By: Claude Opus 5.5 --- src/nexusformat/nexus/lock.py | 25 ++++++++++++--------- tests/test_locks.py | 42 +++++++++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/src/nexusformat/nexus/lock.py b/src/nexusformat/nexus/lock.py index 7b2bf2e..81ae134 100644 --- a/src/nexusformat/nexus/lock.py +++ b/src/nexusformat/nexus/lock.py @@ -27,13 +27,14 @@ class NXLock: Attributes ---------- - lock_file : str - Name of the lock file. This has the extension `.lock` appended to + lock_file : Path + Path of the lock file. This has the extension `.lock` appended to the name of the locked file. pid : int Current process id. - fd : int - File descriptor of the opened lock file. + fd : int or None + File descriptor of the opened lock file, or None if the lock is not + held by this process. """ def __init__(self, filename, timeout=None, check_interval=1, expiry=28800, @@ -62,6 +63,7 @@ def __init__(self, filename, timeout=None, check_interval=1, expiry=28800, """ from .tree import nxgetconfig + self.fd = None self.filename = Path(filename).resolve() suffix = self.filename.suffix + '.lock' if timeout is None: @@ -84,7 +86,6 @@ def __init__(self, filename, timeout=None, check_interval=1, expiry=28800, self.lock_file = self.filename.with_suffix(suffix) self.pid = os.getpid() self.addr = f"{self.pid}@{socket.gethostname()}" - self.fd = None def __repr__(self): return f"NXLock('{self.filename.name}', pid={self.addr})" @@ -141,7 +142,7 @@ def acquire(self, timeout=None, check_interval=None, expiry=None): if self.is_stale(expiry=expiry): self.clear() initial_attempt = False - time.sleep(check_interval) + time.sleep(max(check_interval, 0.1)) else: self.fd = None raise NXLockException( @@ -156,7 +157,10 @@ def release(self): This will only work if the lock was created by the current process. """ if self.fd is not None: - os.close(self.fd) + try: + os.close(self.fd) + except OSError: + pass try: os.remove(self.lock_file) except OSError: @@ -216,7 +220,7 @@ def wait(self, timeout=None, check_interval=None): check_interval = self.check_interval timeoutend = timeit.default_timer() + timeout while timeoutend > timeit.default_timer(): - time.sleep(check_interval) + time.sleep(max(check_interval, 0.1)) if not self.lock_file.exists(): break else: @@ -225,7 +229,7 @@ def wait(self, timeout=None, check_interval=None): return def is_stale(self, expiry=None): - """Return True if the lock file is older than one day. + """Return True if the lock file is older than `expiry` seconds. If the lock file has been cleared before this check, the function returns False to enable another attempt to acquire it. @@ -238,7 +242,8 @@ def is_stale(self, expiry=None): return False def __enter__(self): - return self.acquire() + self.acquire() + return self def __exit__(self, *args): self.release() diff --git a/tests/test_locks.py b/tests/test_locks.py index d787fc1..9c59016 100644 --- a/tests/test_locks.py +++ b/tests/test_locks.py @@ -3,8 +3,9 @@ import time import pytest -from nexusformat.nexus.tree import (NeXusError, NXentry, NXLock, NXroot, - nxload, nxsetconfig, text) +from nexusformat.nexus.tree import (NeXusError, NXentry, NXLock, + NXLockException, NXroot, nxload, + nxsetconfig, text) def test_lock_creation(tmpdir, field4): @@ -225,6 +226,10 @@ def denied(path): assert lock.fd is None + monkeypatch.undo() + lock.clear() + assert not os.path.exists(lock.lock_file) + def test_lock_clear_survives_removal_error(tmpdir, monkeypatch): """clear() must not propagate a PermissionError from an external lock.""" @@ -240,3 +245,36 @@ def denied(path): monkeypatch.setattr(os, 'remove', denied) lock.clear() + + monkeypatch.undo() + lock.clear() + assert not os.path.exists(lock.lock_file) + + +def test_lock_bad_directory_is_collectable(tmpdir): + """A failed constructor must leave an object that can be released.""" + filename = os.path.join(tmpdir, "file1.nxs") + missing = os.path.join(tmpdir, "missing") + + with pytest.raises(NXLockException): + NXLock(filename, directory=missing) + + lock = NXLock.__new__(NXLock) + with pytest.raises(NXLockException): + lock.__init__(filename, directory=missing) + + assert lock.fd is None + lock.release() + + +def test_lock_context_manager_returns_lock(tmpdir): + nxsetconfig(lock=2) + filename = os.path.join(tmpdir, "file1.nxs") + + with NXLock(filename) as lock: + assert isinstance(lock, NXLock) + assert lock.locked + assert os.path.exists(lock.lock_file) + + assert not lock.locked + assert not os.path.exists(lock.lock_file)