From 650786c81270889f154c577a3c74abd51108d856 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 1 Jul 2018 00:56:59 -0700 Subject: 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. --- lib/spack/llnl/util/lock.py | 87 +++++++++++++++++--------- lib/spack/spack/test/llnl/util/lock.py | 110 ++++++++++++++++++++++++++------- 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 -- cgit v1.2.3-60-g2f50