Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use monotonic clock. #150

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use monotonic clock. #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d3fa38
Use time.monotonic instead of datetime.now.
wapiflapi Jan 26, 2022
9764f4a
Add option to pass a custom sleep function.
wapiflapi Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions backoff/_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
# coding:utf-8
import datetime
import time
import functools
import asyncio
from datetime import timedelta

from backoff._common import (_init_wait_gen, _maybe_call, _next_wait)

Expand Down Expand Up @@ -41,6 +40,8 @@ def retry_predicate(target, wait_gen, predicate,
*,
max_tries, max_time, jitter,
on_success, on_backoff, on_giveup,
monotonic_time=None,
sleep=None,
Comment on lines +43 to +44
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random dude passing through here, but IMO it would be cleaner to just declare monotonic_time=time.monotonic, sleep=asyncio.sleep and then just use monotonic_time() and sleep(n) everywhere instead of the (monotonic_time or time.monotonic)() and (sleep or asyncio.sleep)(seconds) calls that lack legibility and clarity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree with you that it would be cleaner. If no one has issues with having the interface be explicit about the default functions that are being used I'm happy with changing that.

If we want to keep having None to indicate "use whatever default the implementation thinks is best" we could have something like sleep_function = time.sleep if sleep is None else sleep somewhere and use that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with the Zen of Python on this: Explicit is better than implicit. Users shouldn't have to guess what the default is when it can be right there in the definition.

The only reason to use None as a default argument and override it with the actual default in the body of the function is if your default is mutable and could lead to unexpected behaviour, such as a dict or list. That's the only case I know of where it's standard to go arg=None and then set the actual default in the body:

if arg is None:
    arg = {}

Unless I'm missing some specifics here of course.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at changing this I remember why i did it like this. The unit tests monkeypatch time.sleep in a lot of places, and if you pass it as a default value to the functions then it makes monkey patching much harder because it needs to be done before the module loads instead of simply before calling the function that is going to use time.sleep.

I am going to have to refactor most unit tests to pass the sleep argument instead of monkey patching.

wait_gen_kwargs):
on_success = _ensure_coroutines(on_success)
on_backoff = _ensure_coroutines(on_backoff)
Expand All @@ -61,11 +62,11 @@ async def retry(*args, **kwargs):
max_time = _maybe_call(max_time)

tries = 0
start = datetime.datetime.now()
start = (monotonic_time or time.monotonic)()
wait = _init_wait_gen(wait_gen, wait_gen_kwargs)
while True:
tries += 1
elapsed = timedelta.total_seconds(datetime.datetime.now() - start)
elapsed = (monotonic_time or time.monotonic)() - start
details = {
"target": target,
"args": args,
Expand Down Expand Up @@ -102,7 +103,7 @@ async def retry(*args, **kwargs):
# See for details:
# <https://groups.google.com/forum/#!topic/python-tulip/yF9C-rFpiKk>
# <https://bugs.python.org/issue28613>
await asyncio.sleep(seconds)
await (sleep or asyncio.sleep)(seconds)
continue
else:
await _call_handlers(on_success, **details, value=ret)
Expand All @@ -117,6 +118,8 @@ def retry_exception(target, wait_gen, exception,
*,
max_tries, max_time, jitter, giveup,
on_success, on_backoff, on_giveup, raise_on_giveup,
sleep=None,
monotonic_time=None,
wait_gen_kwargs):
on_success = _ensure_coroutines(on_success)
on_backoff = _ensure_coroutines(on_backoff)
Expand All @@ -136,11 +139,11 @@ async def retry(*args, **kwargs):
max_time = _maybe_call(max_time)

tries = 0
start = datetime.datetime.now()
start = (monotonic_time or time.monotonic)()
wait = _init_wait_gen(wait_gen, wait_gen_kwargs)
while True:
tries += 1
elapsed = timedelta.total_seconds(datetime.datetime.now() - start)
elapsed = (monotonic_time or time.monotonic)() - start
details = {
"target": target,
"args": args,
Expand Down Expand Up @@ -180,7 +183,7 @@ async def retry(*args, **kwargs):
# See for details:
# <https://groups.google.com/forum/#!topic/python-tulip/yF9C-rFpiKk>
# <https://bugs.python.org/issue28613>
await asyncio.sleep(seconds)
await (sleep or asyncio.sleep)(seconds)
else:
await _call_handlers(on_success, **details)

Expand Down
4 changes: 4 additions & 0 deletions backoff/_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ def on_predicate(wait_gen: _WaitGenerator,
on_success: Union[_Handler, Iterable[_Handler]] = None,
on_backoff: Union[_Handler, Iterable[_Handler]] = None,
on_giveup: Union[_Handler, Iterable[_Handler]] = None,
monotonic_time: Optional[Callable[[], float]] = None,
sleep: Optional[Callable[[float], None]] = None,
logger: _MaybeLogger = 'backoff',
backoff_log_level: int = logging.INFO,
giveup_log_level: int = logging.ERROR,
Expand Down Expand Up @@ -113,6 +115,8 @@ def decorate(target):
on_success=on_success,
on_backoff=on_backoff,
on_giveup=on_giveup,
monotonic_time=monotonic_time,
sleep=sleep,
wait_gen_kwargs=wait_gen_kwargs
)

Expand Down
18 changes: 10 additions & 8 deletions backoff/_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# coding:utf-8
import datetime
import functools
import time
from datetime import timedelta

from backoff._common import (_init_wait_gen, _maybe_call, _next_wait)

Expand All @@ -24,6 +22,8 @@ def retry_predicate(target, wait_gen, predicate,
*,
max_tries, max_time, jitter,
on_success, on_backoff, on_giveup,
monotonic_time=None,
sleep=None,
wait_gen_kwargs):

@functools.wraps(target)
Expand All @@ -35,11 +35,11 @@ def retry(*args, **kwargs):
max_time = _maybe_call(max_time)

tries = 0
start = datetime.datetime.now()
start = (monotonic_time or time.monotonic)()
wait = _init_wait_gen(wait_gen, wait_gen_kwargs)
while True:
tries += 1
elapsed = timedelta.total_seconds(datetime.datetime.now() - start)
elapsed = (monotonic_time or time.monotonic)() - start
details = {
"target": target,
"args": args,
Expand Down Expand Up @@ -67,7 +67,7 @@ def retry(*args, **kwargs):
_call_handlers(on_backoff, **details,
value=ret, wait=seconds)

time.sleep(seconds)
(sleep or time.sleep)(seconds)
continue
else:
_call_handlers(on_success, **details, value=ret)
Expand All @@ -82,6 +82,8 @@ def retry_exception(target, wait_gen, exception,
*,
max_tries, max_time, jitter, giveup,
on_success, on_backoff, on_giveup, raise_on_giveup,
monotonic_time=None,
sleep=None,
wait_gen_kwargs):

@functools.wraps(target)
Expand All @@ -93,11 +95,11 @@ def retry(*args, **kwargs):
max_time = _maybe_call(max_time)

tries = 0
start = datetime.datetime.now()
start = (monotonic_time or time.monotonic)()
wait = _init_wait_gen(wait_gen, wait_gen_kwargs)
while True:
tries += 1
elapsed = timedelta.total_seconds(datetime.datetime.now() - start)
elapsed = (monotonic_time or time.monotonic)() - start
details = {
"target": target,
"args": args,
Expand Down Expand Up @@ -127,7 +129,7 @@ def retry(*args, **kwargs):

_call_handlers(on_backoff, **details, wait=seconds)

time.sleep(seconds)
(sleep or time.sleep)(seconds)
else:
_call_handlers(on_success, **details)

Expand Down
Loading