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.

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

Conversation

wapiflapi
Copy link

@wapiflapi wapiflapi commented Jan 26, 2022

This fixes #149 and #125.

This pull request proposes the use of time.monotonic by default instead of datatime.now. A monotonic clock has the garantee of never going back in time, which is what we want when computing elapsed time.

This also has the benefit of cleaning up tests, because we can easily patch both time.sleep and time.monotonic so that the latter moves forward by exactly the amount of seconds that we pretend-slept for.

This found a "bug" in one of the test which I changed, it requires special attention when reviewing this code. The amount of expect calls to sleep didn't seem logical to me and I believe tthey where the result of time imprecisions.

@wapiflapi wapiflapi changed the title Feature/monotonic Use monotonic time. Jan 26, 2022
@wapiflapi wapiflapi changed the title Use monotonic time. Use monotonic clock. Jan 26, 2022
@bgreen-litl
Copy link
Member

Thanks for this! It's going to take me a little bit to review it carefully, but I will do so as soon as I get a chance.

Comment on lines +43 to +44
monotonic_time=None,
sleep=None,

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.

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.

Cynicjon added a commit to Cynicjon/backoff that referenced this pull request May 21, 2024
Fix to allow trio event loop sleep.
Cynicjon added a commit to Cynicjon/backoff that referenced this pull request May 21, 2024
Fix to allow trio event loop sleep.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use monotonic time
3 participants