openstack/swift
Code Issues Proposed changes

Require lock_path limit to be a positive int

Browse Source 
Currently if limit=0 is passed to lock_path then the method will time
out and never acquire a lock which is reasonable but not useful. This
is a potential pitfall given that in other contexts, for example
diskfile's replication_lock, a concurrency value of 0 has the meaning
'no limit'. It would be easy to erroneously assume that the same
semantic holds for lock_path.

To avoid that pitfall, this patch makes it an error to pass limit<1 to
lock_path.

Related-Change: I3c3193344c7a57a8a4fc7932d1b10e702efd3572
Change-Id: I9ea7ee5b93e3d6924bff9790141b107b53f77883
This commit is contained in:
Alistair Coles 2017-10-24 16:06:26 +01:00
parent e0244dd30d
commit 93e0287a82
2 changed files with 24 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
swift/common/utils.py
View File

@ -2338,7 +2338,11 @@ def lock_path(directory, timeout=10, timeout_class=None, limit=1):
the same directory at the time this method is called. Note that this
limit is only applied during the current call to this method and does
not prevent subsequent calls giving a larger limit. Defaults to 1.
:raises TypeError: if limit is not an int.
:raises ValueError: if limit is less than 1.
"""
if limit < 1:
raise ValueError('limit must be greater than or equal to 1')
if timeout_class is None:
timeout_class = swift.common.exceptions.LockTimeout
mkdirs(directory)

20
test/unit/common/test_utils.py
View File

@ -963,6 +963,26 @@ class TestUtils(unittest.TestCase):
success = True
self.assertFalse(success)
@with_tempdir
def test_lock_path_invalid_limit(self, tmpdir):
success = False
with self.assertRaises(ValueError):
with utils.lock_path(tmpdir, 0.1, limit=0):
success = True
self.assertFalse(success)
with self.assertRaises(ValueError):
with utils.lock_path(tmpdir, 0.1, limit=-1):
success = True
self.assertFalse(success)
with self.assertRaises(TypeError):
with utils.lock_path(tmpdir, 0.1, limit='1'):
success = True
self.assertFalse(success)
with self.assertRaises(TypeError):
with utils.lock_path(tmpdir, 0.1, limit=1.1):
success = True
self.assertFalse(success)
@with_tempdir
def test_lock_path_num_sleeps(self, tmpdir):
num_short_calls = [0]