zhangyiss/spack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

locks: improve errors and permission checking

Browse Source 
- 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.
This commit is contained in:
Todd Gamblin 2018-07-01 00:56:59 -07:00
parent ab794fa741
commit 650786c812
2 changed files with 145 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

87
lib/spack/llnl/util/lock.py
View File

@ -32,7 +32,8 @@
__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 @@ def __init__(self, path, start=0, length=0, debug=False):
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 @@ def _lock(self, op, timeout=_default_timeout):
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:
# 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:
# 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)
# 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 @@ def _lock(self, op, timeout=_default_timeout):
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 @@ def __exit__(self, type, value, traceback):
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 @@ def _exit(self):
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 @@ def _exit(self):
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)

110
lib/spack/spack/test/llnl/util/lock.py
View File

@ -128,12 +128,27 @@
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 p1(barrier):
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 p1(barrier):
# 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 p1(barrier):
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 p2(barrier):
# 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 p2(barrier):
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 p3(barrier):
# 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 p3(barrier):
# 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 p2(barrier, q1, q2):
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