Harden NXLock release and construction

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

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)