summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTodd Gamblin <tgamblin@llnl.gov>2018-07-01 00:56:59 -0700
committerTodd Gamblin <tgamblin@llnl.gov>2018-07-12 19:59:53 +0200
commit650786c81270889f154c577a3c74abd51108d856 (patch)
treef768da1f2ba1fb6878a21bc840b581f6cda2ea47
parentab794fa7413d1650c1ba01ea18e74964877fdf99 (diff)
downloadspack-650786c81270889f154c577a3c74abd51108d856.tar.gz
spack-650786c81270889f154c577a3c74abd51108d856.tar.bz2
spack-650786c81270889f154c577a3c74abd51108d856.tar.xz
spack-650786c81270889f154c577a3c74abd51108d856.zip
locks: improve errors and permission checking
- Clean up error messages for when a lock can't be created, or when an exclusive (write) lock can't be taken on a file. - Add a number of subclasses of LockError to distinguish timeouts from permission issues. - Add an explicit check to prevent the user from taking a write lock on a read-only file. - We had a check for this for when we try to *upgrade* a lock on an RO file, but not for an initial write lock attempt. - Add more tests for different lock permission scenarios.
-rw-r--r--lib/spack/llnl/util/lock.py87
-rw-r--r--lib/spack/spack/test/llnl/util/lock.py110
2 files changed, 145 insertions, 52 deletions
diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py
index 794b201668..10e552bdb5 100644
--- a/lib/spack/llnl/util/lock.py
+++ b/lib/spack/llnl/util/lock.py
@@ -32,7 +32,8 @@ import llnl.util.tty as tty
__all__ = ['Lock', 'LockTransaction', 'WriteTransaction', 'ReadTransaction',
- 'LockError']
+ 'LockError', 'LockTimeoutError',
+ 'LockPermissionError', 'LockROFileError', 'CantCreateLockError']
# Default timeout in seconds, after which locks will raise exceptions.
@@ -80,7 +81,7 @@ class Lock(object):
self.host = self.old_host = None
def _lock(self, op, timeout=_default_timeout):
- """This takes a lock using POSIX locks (``fnctl.lockf``).
+ """This takes a lock using POSIX locks (``fcntl.lockf``).
The lock is implemented as a spin lock using a nonblocking call
to ``lockf()``.
@@ -91,32 +92,36 @@ class Lock(object):
If the lock times out, it raises a ``LockError``.
"""
+ assert op in (fcntl.LOCK_SH, fcntl.LOCK_EX)
+
start_time = time.time()
while (time.time() - start_time) < timeout:
- try:
- # If we could write the file, we'd have opened it 'r+'.
- # Raise an error when we attempt to upgrade to a write lock.
- if op == fcntl.LOCK_EX:
- if self._file and self._file.mode == 'r':
- raise LockError(
- "Can't take exclusive lock on read-only file: %s"
- % self.path)
-
- # Create file and parent directories if they don't exist.
- if self._file is None:
- self._ensure_parent_directory()
-
- # Prefer to open 'r+' to allow upgrading to write
- # lock later if possible. Open read-only if we can't
- # write the lock file at all.
- os_mode, fd_mode = (os.O_RDWR | os.O_CREAT), 'r+'
- if os.path.exists(self.path) and not os.access(
- self.path, os.W_OK):
- os_mode, fd_mode = os.O_RDONLY, 'r'
-
- fd = os.open(self.path, os_mode)
- self._file = os.fdopen(fd, fd_mode)
+ # Create file and parent directories if they don't exist.
+ if self._file is None:
+ parent = self._ensure_parent_directory()
+
+ # Open writable files as 'r+' so we can upgrade to write later
+ os_mode, fd_mode = (os.O_RDWR | os.O_CREAT), 'r+'
+ if os.path.exists(self.path):
+ if not os.access(self.path, os.W_OK):
+ if op == fcntl.LOCK_SH:
+ # can still lock read-only files if we open 'r'
+ os_mode, fd_mode = os.O_RDONLY, 'r'
+ else:
+ raise LockROFileError(self.path)
+
+ elif not os.access(parent, os.W_OK):
+ raise CantCreateLockError(self.path)
+
+ fd = os.open(self.path, os_mode)
+ self._file = os.fdopen(fd, fd_mode)
+
+ elif op == fcntl.LOCK_EX and self._file.mode == 'r':
+ # Attempt to upgrade to write lock w/a read-only file.
+ # If the file were writable, we'd have opened it 'r+'
+ raise LockROFileError(self.path)
+ try:
# Try to get the lock (will raise if not available.)
fcntl.lockf(self._file, op | fcntl.LOCK_NB,
self._length, self._start, os.SEEK_SET)
@@ -138,20 +143,21 @@ class Lock(object):
pass
else:
raise
+
time.sleep(_sleep_time)
- raise LockError("Timed out waiting for lock.")
+ raise LockTimeoutError("Timed out waiting for lock.")
def _ensure_parent_directory(self):
parent = os.path.dirname(self.path)
try:
os.makedirs(parent)
- return True
except OSError as e:
# makedirs can fail when diretory already exists.
if not (e.errno == errno.EEXIST and os.path.isdir(parent) or
e.errno == errno.EISDIR):
raise
+ return parent
def _read_debug_data(self):
"""Read PID and host data out of the file if it is there."""
@@ -340,7 +346,7 @@ class LockTransaction(object):
class ReadTransaction(LockTransaction):
-
+ """LockTransaction context manager that does a read and releases it."""
def _enter(self):
return self._lock.acquire_read(self._timeout)
@@ -349,7 +355,7 @@ class ReadTransaction(LockTransaction):
class WriteTransaction(LockTransaction):
-
+ """LockTransaction context manager that does a write and releases it."""
def _enter(self):
return self._lock.acquire_write(self._timeout)
@@ -358,4 +364,27 @@ class WriteTransaction(LockTransaction):
class LockError(Exception):
+ """Raised for any errors related to locks."""
+
+
+class LockTimeoutError(LockError):
"""Raised when an attempt to acquire a lock times out."""
+
+
+class LockPermissionError(LockError):
+ """Raised when there are permission issues with a lock."""
+
+
+class LockROFileError(LockPermissionError):
+ """Tried to take an exclusive lock on a read-only file."""
+ def __init__(self, path):
+ msg = "Can't take write lock on read-only file: %s" % path
+ super(LockROFileError, self).__init__(msg)
+
+
+class CantCreateLockError(LockPermissionError):
+ """Attempt to create a lock in an unwritable location."""
+ def __init__(self, path):
+ msg = "cannot create lock '%s': " % path
+ msg += "file does not exist and location is not writable"
+ super(LockError, self).__init__(msg)
diff --git a/lib/spack/spack/test/llnl/util/lock.py b/lib/spack/spack/test/llnl/util/lock.py
index 5127462c58..7401ecdc2d 100644
--- a/lib/spack/spack/test/llnl/util/lock.py
+++ b/lib/spack/spack/test/llnl/util/lock.py
@@ -128,12 +128,27 @@ This may need to be higher for some filesystems."""
lock_fail_timeout = 0.1
+def make_readable(*paths):
+ for path in paths:
+ mode = 0o555 if os.path.isdir(path) else 0o444
+ os.chmod(path, mode)
+
+
+def make_writable(*paths):
+ for path in paths:
+ mode = 0o755 if os.path.isdir(path) else 0o744
+ os.chmod(path, mode)
+
+
@contextmanager
-def read_only(path):
- orginal_mode = os.stat(path).st_mode
- os.chmod(path, 0o444)
+def read_only(*paths):
+ modes = [os.stat(p).st_mode for p in paths]
+ make_readable(*paths)
+
yield
- os.chmod(path, orginal_mode)
+
+ for path, mode in zip(paths, modes):
+ os.chmod(path, mode)
@pytest.fixture(scope='session', params=locations)
@@ -172,6 +187,7 @@ def lock_dir(lock_test_directory):
comm.barrier()
if not mpi or comm.rank == 0:
+ make_writable(tempdir)
shutil.rmtree(tempdir)
@@ -188,6 +204,7 @@ def private_lock_path(lock_dir):
yield lock_file
if os.path.exists(lock_file):
+ make_writable(lock_dir, lock_file)
os.unlink(lock_file)
@@ -199,6 +216,7 @@ def lock_path(lock_dir):
yield lock_file
if os.path.exists(lock_file):
+ make_writable(lock_dir, lock_file)
os.unlink(lock_file)
@@ -290,7 +308,7 @@ def timeout_write(lock_path, start=0, length=0):
def fn(barrier):
lock = lk.Lock(lock_path, start, length)
barrier.wait() # wait for lock acquire in first process
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
barrier.wait()
return fn
@@ -300,7 +318,7 @@ def timeout_read(lock_path, start=0, length=0):
def fn(barrier):
lock = lk.Lock(lock_path, start, length)
barrier.wait() # wait for lock acquire in first process
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_read(lock_fail_timeout)
barrier.wait()
return fn
@@ -540,9 +558,47 @@ def test_write_lock_timeout_with_multiple_readers_3_2_ranges(lock_path):
timeout_write(lock_path, 5, 1))
-#
-# Test that read can be upgraded to write.
-#
+def test_read_lock_on_read_only_lockfile(lock_dir, lock_path):
+ """read-only directory, read-only lockfile."""
+ touch(lock_path)
+ with read_only(lock_path, lock_dir):
+ lock = lk.Lock(lock_path)
+
+ with lk.ReadTransaction(lock):
+ pass
+
+ with pytest.raises(lk.LockROFileError):
+ with lk.WriteTransaction(lock):
+ pass
+
+
+def test_read_lock_read_only_dir_writable_lockfile(lock_dir, lock_path):
+ """read-only directory, writable lockfile."""
+ touch(lock_path)
+ with read_only(lock_dir):
+ lock = lk.Lock(lock_path)
+
+ with lk.ReadTransaction(lock):
+ pass
+
+ with lk.WriteTransaction(lock):
+ pass
+
+
+def test_read_lock_no_lockfile(lock_dir, lock_path):
+ """read-only directory, no lockfile (so can't create)."""
+ with read_only(lock_dir):
+ lock = lk.Lock(lock_path)
+
+ with pytest.raises(lk.CantCreateLockError):
+ with lk.ReadTransaction(lock):
+ pass
+
+ with pytest.raises(lk.CantCreateLockError):
+ with lk.WriteTransaction(lock):
+ pass
+
+
def test_upgrade_read_to_write(private_lock_path):
"""Test that a read lock can be upgraded to a write lock.
@@ -597,7 +653,7 @@ def test_upgrade_read_to_write_fails_with_readonly_file(private_lock_path):
assert lock._file.mode == 'r'
# upgrade to writ here
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockROFileError):
lock.acquire_write()
@@ -615,7 +671,7 @@ def test_complex_acquire_and_release_chain(lock_path):
barrier.wait() # ---------------------------------------- 2
lock.release_write() # release and others acquire read
barrier.wait() # ---------------------------------------- 3
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
lock.acquire_read()
barrier.wait() # ---------------------------------------- 4
@@ -624,9 +680,9 @@ def test_complex_acquire_and_release_chain(lock_path):
# p2 upgrades read to write
barrier.wait() # ---------------------------------------- 6
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_read(lock_fail_timeout)
barrier.wait() # ---------------------------------------- 7
# p2 releases write and read
@@ -636,9 +692,9 @@ def test_complex_acquire_and_release_chain(lock_path):
barrier.wait() # ---------------------------------------- 9
# p3 upgrades read to write
barrier.wait() # ---------------------------------------- 10
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_read(lock_fail_timeout)
barrier.wait() # ---------------------------------------- 11
# p3 releases locks
@@ -652,9 +708,9 @@ def test_complex_acquire_and_release_chain(lock_path):
# p1 acquires write
barrier.wait() # ---------------------------------------- 1
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_read(lock_fail_timeout)
barrier.wait() # ---------------------------------------- 2
lock.acquire_read()
@@ -676,9 +732,9 @@ def test_complex_acquire_and_release_chain(lock_path):
barrier.wait() # ---------------------------------------- 9
# p3 upgrades read to write
barrier.wait() # ---------------------------------------- 10
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_read(lock_fail_timeout)
barrier.wait() # ---------------------------------------- 11
# p3 releases locks
@@ -692,9 +748,9 @@ def test_complex_acquire_and_release_chain(lock_path):
# p1 acquires write
barrier.wait() # ---------------------------------------- 1
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_read(lock_fail_timeout)
barrier.wait() # ---------------------------------------- 2
lock.acquire_read()
@@ -706,9 +762,9 @@ def test_complex_acquire_and_release_chain(lock_path):
# p2 upgrades read to write
barrier.wait() # ---------------------------------------- 6
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_write(lock_fail_timeout)
- with pytest.raises(lk.LockError):
+ with pytest.raises(lk.LockTimeoutError):
lock.acquire_read(lock_fail_timeout)
barrier.wait() # ---------------------------------------- 7
# p2 releases write & read
@@ -983,3 +1039,11 @@ def test_lock_debug_output(lock_path):
q1, q2 = Queue(), Queue()
local_multiproc_test(p2, p1, extra_args=(q1, q2))
+
+
+def test_lock_with_no_parent_directory(tmpdir):
+ """Make sure locks work even when their parent directory does not exist."""
+ with tmpdir.as_cwd():
+ lock = lk.Lock('foo/bar/baz/lockfile')
+ with lk.WriteTransaction(lock):
+ pass