From ea10e3bab04a7bc89d86cf2f56a32d08ab4e3b21 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sat, 20 Aug 2016 16:36:40 -0700 Subject: [PATCH] Remove need to touch lock files before using. - Locks will now create enclosing directories and touch the lock file automatically. --- lib/spack/llnl/util/lock.py | 72 +++++++++++++++++++++++++++++------ lib/spack/spack/database.py | 3 -- lib/spack/spack/file_cache.py | 5 +-- lib/spack/spack/package.py | 13 +------ lib/spack/spack/stage.py | 9 +---- lib/spack/spack/test/lock.py | 4 +- 6 files changed, 68 insertions(+), 38 deletions(-) diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py index ce31a59d62..3ebbf25eb8 100644 --- a/lib/spack/llnl/util/lock.py +++ b/lib/spack/llnl/util/lock.py @@ -52,12 +52,16 @@ class Lock(object): """ - def __init__(self, file_path): - self._file_path = file_path + def __init__(self, path): + self.path = path self._file = None self._reads = 0 self._writes = 0 + # PID and host of lock holder + self.pid = self.old_pid = None + self.host = self.old_host = None + def _lock(self, op, timeout): """This takes a lock using POSIX locks (``fnctl.lockf``). @@ -83,15 +87,25 @@ def _lock(self, op, timeout): # Open reader locks read-only if possible. # lock doesn't exist, open RW + create if it doesn't exist. if self._file is None: - mode = 'r+' if op == fcntl.LOCK_EX else 'r' - self._file = open(self._file_path, mode) + self._ensure_parent_directory() + os_mode, fd_mode = os.O_RDONLY, 'r' + if op == fcntl.LOCK_EX or not os.path.exists(self.path): + os_mode, fd_mode = (os.O_RDWR | os.O_CREAT), '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) + + # All locks read the owner PID and host + self._read_lock_data() + + # Exclusive locks write their PID/host if op == fcntl.LOCK_EX: - self._file.write( - "pid=%s,host=%s" % (os.getpid(), socket.getfqdn())) - self._file.truncate() - self._file.flush() + self._write_lock_data() + return except IOError as error: @@ -103,6 +117,40 @@ def _lock(self, op, timeout): raise LockError("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 + + def _read_lock_data(self): + """Read PID and host data out of the file if it is there.""" + line = self._file.read() + if line: + pid, host = line.strip().split(',') + _, _, self.pid = pid.rpartition('=') + _, _, self.host = host.rpartition('=') + + def _write_lock_data(self): + """Write PID and host data to the file, recording old values.""" + self.old_pid = self.pid + self.old_host = self.host + + self.pid = os.getpid() + self.host = socket.getfqdn() + + # write pid, host to disk to sync over FS + self._file.seek(0) + self._file.write("pid=%s,host=%s" % (self.pid, self.host)) + self._file.truncate() + self._file.flush() + os.fsync(self._file.fileno()) + def _unlock(self): """Releases a lock using POSIX locks (``fcntl.lockf``) @@ -126,7 +174,7 @@ def acquire_read(self, timeout=_default_timeout): """ if self._reads == 0 and self._writes == 0: - tty.debug('READ LOCK : {0._file_path} [Acquiring]'.format(self)) + tty.debug('READ LOCK : {0.path} [Acquiring]'.format(self)) self._lock(fcntl.LOCK_SH, timeout) # can raise LockError. self._reads += 1 return True @@ -146,7 +194,7 @@ def acquire_write(self, timeout=_default_timeout): """ if self._writes == 0: - tty.debug('WRITE LOCK : {0._file_path} [Acquiring]'.format(self)) + tty.debug('WRITE LOCK : {0.path} [Acquiring]'.format(self)) self._lock(fcntl.LOCK_EX, timeout) # can raise LockError. self._writes += 1 return True @@ -167,7 +215,7 @@ def release_read(self): assert self._reads > 0 if self._reads == 1 and self._writes == 0: - tty.debug('READ LOCK : {0._file_path} [Released]'.format(self)) + tty.debug('READ LOCK : {0.path} [Released]'.format(self)) self._unlock() # can raise LockError. self._reads -= 1 return True @@ -188,7 +236,7 @@ def release_write(self): assert self._writes > 0 if self._writes == 1 and self._reads == 0: - tty.debug('WRITE LOCK : {0._file_path} [Released]'.format(self)) + tty.debug('WRITE LOCK : {0.path} [Released]'.format(self)) self._unlock() # can raise LockError. self._writes -= 1 return True diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py index 8c29ceeb27..0af08e9449 100644 --- a/lib/spack/spack/database.py +++ b/lib/spack/spack/database.py @@ -160,9 +160,6 @@ def __init__(self, root, db_dir=None): if not os.path.exists(self._db_dir): mkdirp(self._db_dir) - if not os.path.exists(self._lock_path): - touch(self._lock_path) - # initialize rest of state. self.lock = Lock(self._lock_path) self._data = {} diff --git a/lib/spack/spack/file_cache.py b/lib/spack/spack/file_cache.py index 0a66166fd8..31ae009836 100644 --- a/lib/spack/spack/file_cache.py +++ b/lib/spack/spack/file_cache.py @@ -77,10 +77,7 @@ def _lock_path(self, key): def _get_lock(self, key): """Create a lock for a key, if necessary, and return a lock object.""" if key not in self._locks: - lock_file = self._lock_path(key) - if not os.path.exists(lock_file): - touch(lock_file) - self._locks[key] = Lock(lock_file) + self._locks[key] = Lock(self._lock_path(key)) return self._locks[key] def init_entry(self, key): diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 8c00ff9741..9b1c6b11a2 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -703,17 +703,8 @@ def prefix_lock(self): if self._prefix_lock is None: dirname = join_path(os.path.dirname(self.spec.prefix), '.locks') basename = os.path.basename(self.spec.prefix) - lock_file = join_path(dirname, basename) - - if not os.path.exists(lock_file): - tty.debug('TOUCH FILE : {0}'.format(lock_file)) - try: - os.makedirs(dirname) - except OSError: - pass - touch(lock_file) - - self._prefix_lock = llnl.util.lock.Lock(lock_file) + self._prefix_lock = llnl.util.lock.Lock( + join_path(dirname, basename)) return self._prefix_lock diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 8fcac222a0..f282c3e1c0 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -150,15 +150,10 @@ def __init__( self.keep = keep # File lock for the stage directory - self._lock_file = None self._lock = None if lock: - self._lock_file = join_path(spack.stage_path, self.name + '.lock') - if not os.path.exists(self._lock_file): - directory, _ = os.path.split(self._lock_file) - mkdirp(directory) - touch(self._lock_file) - self._lock = llnl.util.lock.Lock(self._lock_file) + self._lock = llnl.util.lock.Lock( + join_path(spack.stage_path, self.name + '.lock')) def __enter__(self): """ diff --git a/lib/spack/spack/test/lock.py b/lib/spack/spack/test/lock.py index 30b7dbce0e..a3b8a3e11a 100644 --- a/lib/spack/spack/test/lock.py +++ b/lib/spack/spack/test/lock.py @@ -44,7 +44,6 @@ class LockTest(unittest.TestCase): def setUp(self): self.tempdir = tempfile.mkdtemp() self.lock_path = join_path(self.tempdir, 'lockfile') - touch(self.lock_path) def tearDown(self): shutil.rmtree(self.tempdir, ignore_errors=True) @@ -172,14 +171,17 @@ def test_upgrade_read_to_write(self): lock.acquire_read() self.assertTrue(lock._reads == 1) self.assertTrue(lock._writes == 0) + self.assertTrue(lock._file.mode == 'r') lock.acquire_write() self.assertTrue(lock._reads == 1) self.assertTrue(lock._writes == 1) + self.assertTrue(lock._file.mode == 'r+') lock.release_write() self.assertTrue(lock._reads == 1) self.assertTrue(lock._writes == 0) + self.assertTrue(lock._file.mode == 'r+') lock.release_read() self.assertTrue(lock._reads == 0)