Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

## New Features

<!-- Here goes the main new features and examples or instructions on how to use them -->
- An optional `tick_at_start` parameter has been added to `Timer`. When `True`, the timer will trigger immediately after starting, and then wait for the interval before triggering again.

## Bug Fixes

Expand Down
25 changes: 23 additions & 2 deletions src/frequenz/channels/timer.py
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ def __init__( # noqa: DOC503 pylint: disable=too-many-arguments
*,
auto_start: bool = True,
start_delay: timedelta = timedelta(0),
tick_at_start: bool = False,
loop: asyncio.AbstractEventLoop | None = None,
) -> None:
"""Initialize this timer.
Expand All @@ -497,6 +498,13 @@ def __init__( # noqa: DOC503 pylint: disable=too-many-arguments
start_delay: The delay before the timer should start. If `auto_start` is
`False`, an exception is raised. This has microseconds resolution,
anything smaller than a microsecond means no delay.
tick_at_start: When `True`, the timer will trigger immediately after
starting, and then wait for the interval before triggering
again. When `False`, the timer will wait the interval before
triggering for the first time. If `auto_start` is `False` and
this is `True`, an exception is raised. If a `start_delay` is
Comment on lines +504 to +505
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why raising if auto_start is False? What is the issue with making this first tick delayed to when the timer actually starts, like when using start_delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restricted by how it is implemented. But if they need it, users can specify tick_on_start=True in the call to reset, just like with start_delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the issue with making this first tick delayed to when the timer actually starts, like when using start_delay?

No, that's not how start_delay is implemented. It also doesn't work with auto_start=False.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, OK.

specified and this is `True`, the first trigger will be immediately
after the `start_delay`.

Choose a reason for hiding this comment

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

One use-case we have is that you want to trigger immediately, then wait for start_delay and from that point onward trigger each interval. This is not possible still with this addition, correct?

Choose a reason for hiding this comment

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

I wonder whether it should be tick_on_init and the combination of tick_at_init=True and start_delay=X should function like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not possible still with this addition, correct?

No, I remember that use case and this should reduce its complexity and make it readable.

But I feel that this is the right balance, and tick_on_init + tick_on_start would make the API for the Timer too complex.

But we can come back to it. And if we are going to add more tick_on_* policies, maybe we should consider an enum instead, describing when to tick.

Choose a reason for hiding this comment

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

What I meant is replace tick_on_start with tick_on_init because AFAIU the behavior here can be achieved with a tick_on_init but what I outlined above is not possible with a tick_on_start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have only tick_on_init, then someone who wants just tick_on_start will get too many ticks and would need more complicated workarounds than someone who needs tick_on_init but only has tick_on_start.

Choose a reason for hiding this comment

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

Which case do you mean here?

Choose a reason for hiding this comment

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

Discussed in person, we take it in for now and keep potential changes for v2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an item describing the 2.0 plan here: #365

loop: The event loop to use to track time. If `None`,
`asyncio.get_running_loop()` will be used.

Expand All @@ -517,6 +525,9 @@ def __init__( # noqa: DOC503 pylint: disable=too-many-arguments
"`auto_start` must be `True` if a `start_delay` is specified"
)

if tick_at_start is True and auto_start is False:
raise ValueError("`auto_start` must be `True` if `tick_at_start` is `True`")

self._interval: int = _to_microseconds(interval)
"""The time to between timer ticks."""

Expand Down Expand Up @@ -567,7 +578,7 @@ def __init__( # noqa: DOC503 pylint: disable=too-many-arguments
"""

if auto_start:
self.reset(start_delay=start_delay)
self.reset(start_delay=start_delay, tick_at_start=tick_at_start)

@property
def interval(self) -> timedelta:
Expand Down Expand Up @@ -595,6 +606,7 @@ def reset( # noqa: DOC503
*,
interval: timedelta | None = None,
start_delay: timedelta = timedelta(0),
tick_at_start: bool = False,
) -> None:
"""Reset the timer to start timing from now (plus an optional delay).

Expand All @@ -608,6 +620,12 @@ def reset( # noqa: DOC503
interval is kept.
start_delay: The delay before the timer should start. This has microseconds
resolution, anything smaller than a microsecond means no delay.
tick_at_start: When `True`, the timer will trigger immediately after
starting, and then wait for the interval before triggering
again. When `False`, the timer will wait the interval before
triggering for the first time. If a `start_delay` is specified
and this is `True`, the first trigger will be immediately after
the `start_delay`.

Raises:
RuntimeError: If it was called without a running loop.
Expand All @@ -621,7 +639,10 @@ def reset( # noqa: DOC503
if interval is not None:
self._interval = _to_microseconds(interval)

self._next_tick_time = self._now() + start_delay_ms + self._interval
self._next_tick_time = self._now() + start_delay_ms

if not tick_at_start:
self._next_tick_time += self._interval

if self.is_running:
self._reset_event.set()
Expand Down
53 changes: 53 additions & 0 deletions tests/test_timer.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import asyncio
import enum
import re
from datetime import timedelta

import async_solipsism
Expand Down Expand Up @@ -331,6 +332,18 @@ async def test_timer_construction_wrong_args() -> None:
loop=None,
)

with pytest.raises(
ValueError,
match=re.escape("`auto_start` must be `True` if `tick_at_start` is `True`"),
):
_ = Timer(
timedelta(seconds=5.0),
SkipMissedAndResync(),
auto_start=False,
tick_at_start=True,
loop=None,
)


async def test_timer_close_receiver() -> None:
"""Test the autostart of a periodic timer."""
Expand Down Expand Up @@ -384,6 +397,46 @@ async def test_timer_autostart_with_delay() -> None:
assert event_loop.time() == pytest.approx(2.5)


async def test_timer_autostart_with_tick_at_start() -> None:
"""Test the autostart of a periodic timer with a tick at start."""
event_loop = asyncio.get_running_loop()

timer = Timer(timedelta(seconds=1.0), TriggerAllMissed(), tick_at_start=True)

# The first tick should be at 0.0, without any delay.
drift = await timer.receive()
assert drift == pytest.approx(timedelta(seconds=0.0))
assert event_loop.time() == pytest.approx(0.0)

# The next tick should be at 1.0
drift = await timer.receive()
assert drift == pytest.approx(timedelta(seconds=0.0))
assert event_loop.time() == pytest.approx(1.0)


async def test_timer_autostart_with_delay_and_tick_at_start() -> None:
"""Test the autostart of a periodic timer with a start delay and tick at start."""
event_loop = asyncio.get_running_loop()

timer = Timer(
timedelta(seconds=1.0),
TriggerAllMissed(),
tick_at_start=True,
start_delay=timedelta(seconds=0.5),
)

# The first tick should be at 0.5, as soon as the start delay is over.
await asyncio.sleep(0.3)
drift = await timer.receive()
assert drift == pytest.approx(timedelta(seconds=0.0))
assert event_loop.time() == pytest.approx(0.5)

# The next tick should be at 1.5
drift = await timer.receive()
assert drift == pytest.approx(timedelta(seconds=0.0))
assert event_loop.time() == pytest.approx(1.5)


class _StartMethod(enum.Enum):
RESET = enum.auto()
RECEIVE = enum.auto()
Expand Down
Loading