Repository: python-zk/kazoo
Pull request #748 (2024-03-11 03:02:46)

fix(core): Proper retry count in KazooRetry
Make sure the number of attempts matches the `max_retry` parameter. Add unit tests to that effect.

## Why is this needed?

So that users can precisely control the number of retries

## Proposed Changes

  - Move the location of the attempt increase to before the check against `max_try` so we don't end up calling `max_tries + 1` times the function.

## Does this PR introduce any breaking change?

No, it shouldn't.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/python-zk/kazoo/pull/748?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 96.50%. Comparing base [(`4c6bad8`)](https://app.codecov.io/gh/python-zk/kazoo/commit/4c6bad82ebe347b9e51abbf33cef443057dc1c45?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk) to head [(`81b35dd`)](https://app.codecov.io/gh/python-zk/kazoo/pull/748?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk).
> Report is 7 commits behind head on master.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #748      +/-   ##
==========================================
- Coverage   96.84%   96.50%   -0.34%     
==========================================
  Files          27       27              
  Lines        3549     3549              
==========================================
- Hits         3437     3425      -12     
- Misses        112      124      +12     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/python-zk/kazoo/pull/748?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk).


Patch:
diff --git a/kazoo/retry.py b/kazoo/retry.py
index a51a1b5..fb9e8fc 100644
--- a/kazoo/retry.py
+++ b/kazoo/retry.py
@@ -133,10 +133,10 @@ class KazooRetry(object):
             except ConnectionClosedError:
                 raise
             except self.retry_exceptions:
+                self._attempts += 1
                 # Note: max_tries == -1 means infinite tries.
                 if self._attempts == self.max_tries:
                     raise RetryFailedError("Too many retry attempts")
-                self._attempts += 1
                 jitter = random.uniform(
                     1.0 - self.max_jitter, 1.0 + self.max_jitter
                 )


Test patch:
diff --git a/kazoo/tests/test_retry.py b/kazoo/tests/test_retry.py
index 5b79eca..c26d399 100644
--- a/kazoo/tests/test_retry.py
+++ b/kazoo/tests/test_retry.py
@@ -1,89 +1,103 @@
-import unittest
+from unittest import mock
 
 import pytest
 
+from kazoo import exceptions as ke
+from kazoo import retry as kr
 
-class TestRetrySleeper(unittest.TestCase):
-    def _pass(self):
-        pass
 
-    def _fail(self, times=1):
-        from kazoo.retry import ForceRetryError
+def _make_retry(*args, **kwargs):
+    """Return a KazooRetry instance with a dummy sleep function."""
+
+    def _sleep_func(_time):
+        pass
 
-        scope = dict(times=0)
+    return kr.KazooRetry(*args, sleep_func=_sleep_func, **kwargs)
 
-        def inner():
-            if scope["times"] >= times:
-                pass
-            else:
-                scope["times"] += 1
-                raise ForceRetryError("Failed!")
 
-        return inner
+def _make_try_func(times=1):
+    """Returns a function that raises ForceRetryError `times` time before
+    returning None.
+    """
+    callmock = mock.Mock(
+        side_effect=[kr.ForceRetryError("Failed!")] * times + [None],
+    )
+    return callmock
 
-    def _makeOne(self, *args, **kwargs):
-        from kazoo.retry import KazooRetry
 
-        return KazooRetry(*args, **kwargs)
+def test_call():
+    retry = _make_retry(delay=0, max_tries=2)
+    func = _make_try_func()
+    retry(func, "foo", bar="baz")
+    assert func.call_args_list == [
+        mock.call("foo", bar="baz"),
+        mock.call("foo", bar="baz"),
+    ]
 
-    def test_reset(self):
-        retry = self._makeOne(delay=0, max_tries=2)
-        retry(self._fail())
-        assert retry._attempts == 1
-        retry.reset()
-        assert retry._attempts == 0
 
-    def test_too_many_tries(self):
-        from kazoo.retry import RetryFailedError
+def test_reset():
+    retry = _make_retry(delay=0, max_tries=2)
+    func = _make_try_func()
+    retry(func)
+    assert (
+        func.call_count == retry._attempts + 1 == 2
+    ), "Called 2 times, failed _attempts 1, succeeded 1"
+    retry.reset()
+    assert retry._attempts == 0
 
-        retry = self._makeOne(delay=0)
-        with pytest.raises(RetryFailedError):
-            retry(self._fail(times=999))
-        assert retry._attempts == 1
 
-    def test_maximum_delay(self):
-        def sleep_func(_time):
-            pass
+def test_too_many_tries():
+    retry = _make_retry(delay=0, max_tries=10)
+    func = _make_try_func(times=999)
+    with pytest.raises(kr.RetryFailedError):
+        retry(func)
+    assert (
+        func.call_count == retry._attempts == 10
+    ), "Called 10 times, failed _attempts 10"
 
-        retry = self._makeOne(delay=10, max_tries=100, sleep_func=sleep_func)
-        retry(self._fail(times=10))
-        assert retry._cur_delay < 4000
-        # gevent's sleep function is picky about the type
-        assert type(retry._cur_delay) == float
 
-    def test_copy(self):
-        def _sleep(t):
-            return None
+def test_maximum_delay():
+    retry = _make_retry(delay=10, max_tries=100, max_jitter=0)
+    func = _make_try_func(times=2)
+    retry(func)
+    assert func.call_count == 3, "Called 3 times, 2 failed _attemps"
+    assert retry._cur_delay == 10 * 2**2, "Normal exponential backoff"
+    retry.reset()
+    func = _make_try_func(times=10)
+    retry(func)
+    assert func.call_count == 11, "Called 11 times, 10 failed _attemps"
+    assert retry._cur_delay == 60, "Delay capped by maximun"
+    # gevent's sleep function is picky about the type
+    assert isinstance(retry._cur_delay, float)
 
-        retry = self._makeOne(sleep_func=_sleep)
-        rcopy = retry.copy()
-        assert rcopy.sleep_func is _sleep
 
+def test_copy():
+    retry = _make_retry()
+    rcopy = retry.copy()
+    assert rcopy is not retry
+    assert rcopy.sleep_func is retry.sleep_func
 
-class TestKazooRetry(unittest.TestCase):
-    def _makeOne(self, **kw):
-        from kazoo.retry import KazooRetry
 
-        return KazooRetry(**kw)
+def test_connection_closed():
+    retry = _make_retry()
 
-    def test_connection_closed(self):
-        from kazoo.exceptions import ConnectionClosedError
+    def testit():
+        raise ke.ConnectionClosedError
 
-        retry = self._makeOne()
+    with pytest.raises(ke.ConnectionClosedError):
+        retry(testit)
 
-        def testit():
-            raise ConnectionClosedError()
 
-        with pytest.raises(ConnectionClosedError):
-            retry(testit)
+def test_session_expired():
+    retry = _make_retry(max_tries=1)
 
-    def test_session_expired(self):
-        from kazoo.exceptions import SessionExpiredError
+    def testit():
+        raise ke.SessionExpiredError
 
-        retry = self._makeOne(max_tries=1)
+    with pytest.raises(kr.RetryFailedError):
+        retry(testit)
 
-        def testit():
-            raise SessionExpiredError()
+    retry = _make_retry(max_tries=1, ignore_expire=False)
 
-        with pytest.raises(Exception):
-            retry(testit)
+    with pytest.raises(ke.SessionExpiredError):
+        retry(testit)