From 73309b2d456c8e64f4ceb079f98903fc9e45f3b6 Mon Sep 17 00:00:00 2001 From: Charles-Henri de Boysson Date: Thu, 29 Feb 2024 01:44:58 -0500 Subject: [PATCH] fix(core): Proper retry count in KazooRetry Make sure the number of attempts matches the `max_retry` parameter. Add unit tests to that effect. --- kazoo/retry.py | 2 +- kazoo/tests/test_retry.py | 136 +++++++++++++++++++++----------------- 2 files changed, 76 insertions(+), 62 deletions(-) diff --git a/kazoo/retry.py b/kazoo/retry.py index a51a1b56..fb9e8fc7 100644 --- a/kazoo/retry.py +++ b/kazoo/retry.py @@ -133,10 +133,10 @@ def __call__(self, func, *args, **kwargs): 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 ) diff --git a/kazoo/tests/test_retry.py b/kazoo/tests/test_retry.py index 5b79eca9..c26d3997 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)