From 93e0287a827c0025c1522517b0e288ec04f12326 Mon Sep 17 00:00:00 2001
From: Alistair Coles <alistairncoles@gmail.com>
Date: Tue, 24 Oct 2017 16:06:26 +0100
Subject: [PATCH] Require lock_path limit to be a positive int

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
---
 swift/common/utils.py          |  4 ++++
 test/unit/common/test_utils.py | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/swift/common/utils.py b/swift/common/utils.py
index e49053338f..839789ef78 100644
--- a/swift/common/utils.py
+++ b/swift/common/utils.py
@@ -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)
diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py
index 321cefa40d..9a6c11994f 100644
--- a/test/unit/common/test_utils.py
+++ b/test/unit/common/test_utils.py
@@ -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]