summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTodd Gamblin <tgamblin@llnl.gov>2019-12-21 16:31:28 -0800
committerTodd Gamblin <tgamblin@llnl.gov>2019-12-23 23:18:45 -0800
commitd87ededddce53a8e3ae18ba8f75e206f6e395793 (patch)
tree808cd82ba8aaa9ddfc7ede39acd380993e87c710
parentb3a5f2e3c3ce4e3e9836301504f5b5987117248e (diff)
downloadspack-d87ededddce53a8e3ae18ba8f75e206f6e395793.tar.gz
spack-d87ededddce53a8e3ae18ba8f75e206f6e395793.tar.bz2
spack-d87ededddce53a8e3ae18ba8f75e206f6e395793.tar.xz
spack-d87ededddce53a8e3ae18ba8f75e206f6e395793.zip
lock transactions: avoid redundant reading in write transactions
Our `LockTransaction` class was reading overly aggressively. In cases like this: ``` 1 with spack.store.db.read_transaction(): 2 with spack.store.db.write_transaction(): 3 ... ``` The `ReadTransaction` on line 1 would read in the DB, but the WriteTransaction on line 2 would read in the DB *again*, even though we had a read lock the whole time. `WriteTransaction`s were only considering nested writes to decide when to read, but they didn't know when we already had a read lock. - [x] `Lock.acquire_write()` return `False` in cases where we already had a read lock.
-rw-r--r--lib/spack/llnl/util/lock.py7
-rw-r--r--lib/spack/spack/test/llnl/util/lock.py56
2 files changed, 62 insertions, 1 deletions
diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py
index 86a45e2d7c..3a58093491 100644
--- a/lib/spack/llnl/util/lock.py
+++ b/lib/spack/llnl/util/lock.py
@@ -275,7 +275,12 @@ class Lock(object):
wait_time, nattempts = self._lock(fcntl.LOCK_EX, timeout=timeout)
self._acquired_debug('WRITE LOCK', wait_time, nattempts)
self._writes += 1
- return True
+
+ # return True only if we weren't nested in a read lock.
+ # TODO: we may need to return two values: whether we got
+ # the write lock, and whether this is acquiring a read OR
+ # write lock for the first time. Now it returns the latter.
+ return self._reads == 0
else:
self._writes += 1
return False
diff --git a/lib/spack/spack/test/llnl/util/lock.py b/lib/spack/spack/test/llnl/util/lock.py
index 2b0892a25e..ca879cdc0b 100644
--- a/lib/spack/spack/test/llnl/util/lock.py
+++ b/lib/spack/spack/test/llnl/util/lock.py
@@ -1087,6 +1087,62 @@ def test_nested_write_transaction(lock_path):
assert vals['wrote']
+def test_nested_reads(lock_path):
+ """Ensure that write transactions won't re-read data."""
+
+ def read():
+ vals['read'] += 1
+
+ vals = collections.defaultdict(lambda: 0)
+ lock = AssertLock(lock_path, vals)
+
+ # read/read
+ vals.clear()
+ assert vals['read'] == 0
+ with lk.ReadTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.ReadTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+
+ # write/write
+ vals.clear()
+ assert vals['read'] == 0
+ with lk.WriteTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.WriteTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+
+ # read/write
+ vals.clear()
+ assert vals['read'] == 0
+ with lk.ReadTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.WriteTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+
+ # write/read/write
+ vals.clear()
+ assert vals['read'] == 0
+ with lk.WriteTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.ReadTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.WriteTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+
+ # read/write/read/write
+ vals.clear()
+ assert vals['read'] == 0
+ with lk.ReadTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.WriteTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.ReadTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+ with lk.WriteTransaction(lock, acquire=read):
+ assert vals['read'] == 1
+
+
def test_lock_debug_output(lock_path):
host = socket.getfqdn()