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

Monotonic overhaul #406

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

Mocramis
Copy link

With version 2.13 I noticed that all my tests using time-machine and asyncio were segfaulting. I have added an minimal example of such breaking at the end of this pr.

The cause of the segfault, was a post-module unload call from logging itself called from asyncio exception handler.
The obvious fix was to handle having an unloaded module from _time_machine_time and that is what's the first commit of the serie is about (fa9cc85).

However, the tests still fails after that (but they now complains about an unloaded module rather than crashing).

After a little bissect i found that the culprit commit was the one introduced in #382 : This commit replace monotonic by our custom time.time. This is problematic as many user of monotonic assumes that the returned value is, well, monotonic.

Having a non-monotonic time.monotonic is really problematic and does break a lot of use case. But even more, the Pr did not restore monotonic to it's pre-travel time after popping the coordinates, causing monotonic to be definitively wrong when travel was used once.

This restoration problem is fixed in the second commit (fa9d2ee).

The third commit of the serie (2e2e77b) force monotonic to start from the pre-travel monotonic (to avoid getting to pre-monotonic time or jumping from a the monotonic value to the time value (which are not in the same order at all). This is not an issue as the monotonic value should not hold any meaning, only the difference in value should have an impact. Monotonic still only ticks when a time function is called. It is not fully accurate as it should ideally use the monotonic time as a ticking source and not the normal time, but as it will only break one second every year at most, it is an ok compromise until the way monotonic is handled is properly defined.

This leads us to another issue: since monotonic can only grow, any shift or move_to should not affect it further than the natural ticking time: Indeed, if we jump backward, monotonic will naturally break, but evan if we jump forward it will step back at the end of the travel time.

The fourth commit (d8e3614) changes the current behavior and prevent those actions to affect monotonic unless an explicit affect_monotonic is passed (and the user is responsible for any subsequent breaking).

I think that ideally, monotonic may need to be handled completely differently than time.time, but this would require a lot more work and would need to add specific apis.

Below is a simple example triggering the mentionned crashes (should be run with pytest with the pytest-asyncio plugin):

import asyncio
import datetime

import pytest
from time_machine import travel


pytest_plugins = ('pytest_asyncio',)

tasks = []

async def crash():
    tasks.append(asyncio.create_task(asyncio.sleep(100)))
    with travel(datetime.datetime.now() + datetime.timedelta(minutes=5)):
        pass
    assert False

@pytest.mark.asyncio
async def test_crash():
    task = await crash()

I believe this may fix the problems mentionned in #403 #393 and #387

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.

1 participant