From 6c9467e8c6d5fffa6d9412d5d36ff2c91e33ea21 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sat, 21 Dec 2019 16:31:28 -0800 Subject: 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. --- lib/spack/llnl/util/lock.py | 7 ++++- lib/spack/spack/test/llnl/util/lock.py | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) (limited to 'lib') 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() -- cgit v1.2.3-70-g09d2