Skip to content

Change fail_after&move_on_after to set deadline relative to entering. Add CancelScope.relative_deadline #3010

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

Merged
merged 32 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
dea2e79
flake8-trio has been renamed
jakkdl May 22, 2024
d6e91cc
Add explicit documentation on timeout semantic for fail&move_on_after
jakkdl Jun 6, 2024
dc3bd3d
Merge branch 'master' into fail_after_documentation
jakkdl Jun 6, 2024
da25949
use a5rocks alternate phrasing for move_on_after
jakkdl Jun 7, 2024
7a8262a
add `relative_deadline` attribute to CancelScope. `move_on_after` and…
jakkdl Jun 20, 2024
d028037
Merge branch 'master' into fail_after_documentation
jakkdl Jun 20, 2024
5447f45
fix codecov, mypy now fails to unify return types across the timeouts…
jakkdl Jun 20, 2024
1381560
instead of a breaking change we now raise deprecationwarning and give…
jakkdl Jun 24, 2024
742c013
don't inherit from AbstractContextManager since that breaks 3.8, and …
jakkdl Jun 24, 2024
0075b56
fix RTD build. Replace star import with explicit list. Remove useless…
jakkdl Jun 24, 2024
9d3d0b9
reimplement as transforming class
jakkdl Jun 26, 2024
b29db0d
Merge branch 'master' into fail_after_documentation
jakkdl Jun 27, 2024
480681d
whoops, forgot to actually enter the cancelscope
jakkdl Jun 27, 2024
00ba1ca
Merge remote-tracking branch 'origin/main' into fail_after_documentation
jakkdl Aug 31, 2024
510aaa1
remove unneeded type:ignore with mypy 1.11
jakkdl Aug 31, 2024
7079d71
Merge branch 'main' into fail_after_documentation
jakkdl Sep 5, 2024
3474128
write docs, update newsfragments to match current implementation
jakkdl Sep 5, 2024
df5876d
add transitional functions
jakkdl Sep 5, 2024
a240948
fix test
jakkdl Sep 5, 2024
0183b43
fix tests/codecov
jakkdl Sep 5, 2024
ca63354
fix test
jakkdl Sep 5, 2024
7591297
Merge remote-tracking branch 'origin/main' into repro_pyright_verifyt…
jakkdl Sep 12, 2024
75fc52b
quick re-implementation after new spec. Docs/docstrings have not had …
jakkdl Sep 12, 2024
5a32eb0
clean up return type of fail_at/_after for sphinx
jakkdl Sep 16, 2024
c2a3d8e
remove _is_relative, and calculate it on the fly instead. Add nan han…
jakkdl Sep 16, 2024
f8e9417
change some RuntimeError to DeprecationWarning, fix tests/coverage
jakkdl Sep 16, 2024
36786a6
pragma: no cover
jakkdl Sep 16, 2024
8e863b8
Merge remote-tracking branch 'origin/main' into fail_after_documentation
jakkdl Sep 16, 2024
6a7650d
duplicate validation logic, bump pyright
jakkdl Sep 18, 2024
02ac588
Merge remote-tracking branch 'origin/main' into fail_after_documentation
jakkdl Sep 18, 2024
bf12a80
update docs/newsfragments/docstrings
jakkdl Sep 18, 2024
9961abc
properly link to ASYNC122
jakkdl Sep 19, 2024
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
18 changes: 18 additions & 0 deletions newsfragments/2512.breaking.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
:func:`trio.move_on_after` and :func:`trio.fail_after` now sets the deadline upon entering the context manager, instead of at initialization. The vast majority of usage initialized them when entering and is therefore unaffected. But if you have code that looks like:

.. code-block:: python3

cs = trio._move_on_after(5)
...
with cs:
...


the deadline of the underlying :func:`trio.CancelScope` will now differ. If you want to fully preserve current behaviour you can rewrite it as

.. code-block:: python3

cs = trio.move_on_at(trio.current_time() + 5)
...
with cs:
...
13 changes: 13 additions & 0 deletions newsfragments/2512.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
:func:`trio.CancelScope` now has an attribute ``relative_deadline`` that can be used together with, or instead of, ``deadline``. :ref:`trio.move_on_after` and :ref:`trio.fail_after` now use this functionality in order to resolve the absolute deadline upon entering the context manager.

If setting both ``deadline`` and ``relative_deadline`` before entering the cm, the deadline will be set to ``min(deadline, trio.current_time() + relative_deadline)``.
Accessing ``relative_deadline`` after entering will return remaining time until deadline (i.e. ``deadline - trio.current_time()``. Setting ``relative_deadline`` after entering will set ``deadline`` to ``trio.current_time() + relative_deadline``

Example:

.. code-block:: python3

my_cs = trio.CancelScope(relative_deadline = 5)
...
with my_cs: # my_cs.deadline will now be 5 seconds into the future
...
31 changes: 31 additions & 0 deletions src/trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,11 @@ class CancelScope:

# Constructor arguments:
_deadline: float = attrs.field(default=inf, kw_only=True, alias="deadline")
# use float|None, or math.inf?
# or take it as an opportunity to also introduce deadline=None?
_relative_deadline: float | None = attrs.field(
default=None, kw_only=True, alias="relative_deadline"
)
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 think deadline + relative_deadline should be supported: we could deny that using a post-init thing from attrs. Maybe someone has a use case for it? I can't imagine anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can maybe imagine something, but it feels niche and almost certainly could be recreated with two nested scopes or custom functions.

But if we don't want simultaneous usage, and also don't want relative_deadline usage after entering, then I think we should go with a different approach than adding an attribute to CancelScope. In the linked issue I discussed other approaches, see e.g. "new class that transforms into CancelScope" in #2512 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @Zac-HD because you have opinions.

I think what @Zac-HD proposed of disallowing setting relative deadlines inside the cancel scope would help somewhat, even though I still think that absolute deadlines vs relative deadlines is a weird concept to have before the context manager. (So namely being able to pass both here and being able to set both with their properties)

Copy link
Member

Choose a reason for hiding this comment

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

My preferred semantics are:

  • CancelScope gets a new ._relative_deadline: float property, with no corresponding init argument
  • the *_after() scopes create a CancelScope with deadline=inf, and then set the attribute
  • on entry to the CancelScope, set the deadline to min(deadline, _relative_deadline + current_time()), and then set _relative_deadline = inf
    • this means that you get the same semantics for setting deadline as any other scope, but "relative deadlines" don't become a semantic thing for users to think about
  • it's an error to set the _relative_deadline if the scope has ever been entered

The existing deprecation logic is a bit ugly, but seems like the best option available overall.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this your preferred semantics? I'm mostly just seeing downsides to having the feature be this hidden, but still able to sneakily introduce confusing behavior if a user is sending around a CancelScope. If I have a function that takes a CancelScope as a parameter, I'd have to introspect the hidden attribute if I wanted to figure out what it would set the deadline to once entered. Maybe that's uncommon logic-wise, but surely people are writing logging statements like print(f"this cs will time out at {cs.deadline}") (before entering).

If we don't want to introduce relative deadlines as a primary feature, and avoid any stealthy gotchas, then I strongly think we should introduce a class that transforms into a CancelScope - to make sure that typing would catch any attempt at passing/saving the return value of fail/move_on_after to something that doesn't expect precisely that, and raise AttributeError if attempting to access deadline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the min variant also breaks trying to change the deadline, unless we unset relative_deadline upon setting deadline. So ultimately there's no reason to use min and we instead do some conditionals on whether values are None/inf.

cs = move_on_after(5)
# wait no I want to make the deadline later
cs.deadline = trio.current_time() + 7
with cs:
    print(cs.deadline - trio.current_time()) # prints 5

this is regardless of if the time is relative to construction or entering.

and of course

cs = move_on_after(5)
cs.deadline = cs.deadline + 2
with cs:
    print(cs.deadline)  # prints 5, not 7, which it would previously

Copy link
Member

Choose a reason for hiding this comment

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

I now agree that my proposal upthread is clearly bad 😅

_shield: bool = attrs.field(default=False, kw_only=True, alias="shield")

@enable_ki_protection
Expand All @@ -550,6 +555,13 @@ def __enter__(self) -> Self:
"Each CancelScope may only be used for a single 'with' block"
)
self._has_been_entered = True

if self._relative_deadline is not None:
if self._relative_deadline + current_time() < self._deadline:
self._deadline = self._relative_deadline + current_time()
else:
self._relative_deadline = self.deadline - current_time()

if current_time() >= self._deadline:
self.cancel()
with self._might_change_registered_deadline():
Expand Down Expand Up @@ -744,6 +756,25 @@ def deadline(self, new_deadline: float) -> None:
with self._might_change_registered_deadline():
self._deadline = float(new_deadline)

@property
def relative_deadline(self) -> float | None:
"""TODO: write docstring"""
if self._has_been_entered:
if self.deadline == inf:
return None
return self._deadline - current_time()
return self._relative_deadline

@relative_deadline.setter
def relative_deadline(self, relative_deadline: float | None) -> None:
self._relative_deadline = relative_deadline
if self._has_been_entered:
with self._might_change_registered_deadline():
if relative_deadline is None:
self._deadline = inf
else:
self._deadline = current_time() + relative_deadline

@property
def shield(self) -> bool:
"""Read-write, :class:`bool`, default :data:`False`. So long as
Expand Down
43 changes: 43 additions & 0 deletions src/trio/_core/_tests/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,49 @@ async def test_basic_timeout(mock_clock: _core.MockClock) -> None:
await _core.checkpoint()


async def test_relative_timeout(mock_clock: _core.MockClock) -> None:
# test defaults
scope = _core.CancelScope()
assert scope.deadline == inf
assert scope.relative_deadline is None

# setting relative_deadline before entering does not modify deadline
scope.relative_deadline = 1
assert scope.deadline == inf
assert scope.relative_deadline == 1

# check that setting either deadline updates the other (after entering)
start = _core.current_time()
with scope:
assert scope.deadline == start + 1
scope.relative_deadline = 2
assert scope.deadline == start + 2
scope.deadline = start + 3
assert scope.relative_deadline == 3

# relative_deadline < deadline, deadline is updated
with _core.CancelScope(relative_deadline=1, deadline=start + 2) as scope:
assert scope.deadline == start + 1
assert scope.relative_deadline == 1

# deadline < relative_deadline, relative_deadline is updated
with _core.CancelScope(relative_deadline=2, deadline=start + 1):
assert scope.deadline == start + 1
assert scope.relative_deadline == 1

# once entered, accessing relative_deadline returns deadline-now()
# and setting it calculates it relative to now()
start = _core.current_time()
with _core.CancelScope(relative_deadline=3) as scope:
mock_clock.jump(1)
assert scope.relative_deadline == 2
mock_clock.jump(1)
assert scope.relative_deadline == 1
scope.relative_deadline = 3
mock_clock.jump(1)
assert scope.relative_deadline == 2


async def test_cancel_scope_nesting() -> None:
# Nested scopes: if two triggering at once, the outer one wins
with _core.CancelScope() as scope1:
Expand Down
20 changes: 20 additions & 0 deletions src/trio/_tests/test_timeouts.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,23 @@ async def test_timeouts_raise_value_error() -> None:
):
with cm(val):
pass # pragma: no cover


async def test_timeout_deadline_on_entry(mock_clock: _core.MockClock) -> None:
cs = move_on_after(5)
assert cs.relative_deadline == 5
mock_clock.jump(3)
start = _core.current_time()
with cs:
# This would previously be start+2
assert cs.deadline == start + 5
assert cs.relative_deadline == 5

# because fail_after is a wrapping contextmanager we cannot directly inspect
# cs_gen.relative_deadline before entering
cs_gen = fail_after(5)
mock_clock.jump(3)
start = _core.current_time()
with cs_gen as cs:
assert cs.deadline == start + 5
assert cs.relative_deadline == 5
28 changes: 15 additions & 13 deletions src/trio/_timeouts.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
from __future__ import annotations

import math
from contextlib import AbstractContextManager, contextmanager
from contextlib import contextmanager
from typing import TYPE_CHECKING

import trio

if TYPE_CHECKING:
from collections.abc import Generator


def move_on_at(deadline: float) -> trio.CancelScope:
"""Use as a context manager to create a cancel scope with the given
Expand Down Expand Up @@ -36,7 +39,9 @@ def move_on_after(seconds: float) -> trio.CancelScope:
"""
if seconds < 0:
raise ValueError("timeout must be non-negative")
return move_on_at(trio.current_time() + seconds)
if math.isnan(seconds):
raise ValueError("timeout must not be NaN")
return trio.CancelScope(relative_deadline=seconds)


async def sleep_forever() -> None:
Expand Down Expand Up @@ -94,9 +99,8 @@ class TooSlowError(Exception):
"""


# workaround for PyCharm not being able to infer return type from @contextmanager
# see https://youtrack.jetbrains.com/issue/PY-36444/PyCharm-doesnt-infer-types-when-using-contextlib.contextmanager-decorator
def fail_at(deadline: float) -> AbstractContextManager[trio.CancelScope]: # type: ignore[misc]
@contextmanager
def fail_at(deadline: float) -> Generator[trio.CancelScope, None, None]:
"""Creates a cancel scope with the given deadline, and raises an error if it
is actually cancelled.

Expand All @@ -123,11 +127,8 @@ def fail_at(deadline: float) -> AbstractContextManager[trio.CancelScope]: # typ
raise TooSlowError


if not TYPE_CHECKING:
fail_at = contextmanager(fail_at)


def fail_after(seconds: float) -> AbstractContextManager[trio.CancelScope]:
@contextmanager
def fail_after(seconds: float) -> Generator[trio.CancelScope, None, None]:
"""Creates a cancel scope with the given timeout, and raises an error if
it is actually cancelled.

Expand All @@ -150,6 +151,7 @@ def fail_after(seconds: float) -> AbstractContextManager[trio.CancelScope]:
ValueError: if *seconds* is less than zero or NaN.

"""
if seconds < 0:
raise ValueError("timeout must be non-negative")
return fail_at(trio.current_time() + seconds)
with move_on_after(seconds) as scope:
yield scope
if scope.cancelled_caught:
raise TooSlowError