Skip to content
Merged
Show file tree
Hide file tree
Changes from 26 commits
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
4 changes: 2 additions & 2 deletions docs/source/reference-core.rst
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,8 @@ situation of just wanting to impose a timeout on some code:
.. autofunction:: fail_at
:with: cancel_scope

Cheat sheet:
Cheat sheet
+++++++++++

* If you want to impose a timeout on a function, but you don't care
whether it timed out or not:
Expand Down Expand Up @@ -597,7 +598,6 @@ which is sometimes useful:

.. autofunction:: current_effective_deadline


.. _tasks:

Tasks let you do multiple things at once
Expand Down
1 change: 1 addition & 0 deletions newsfragments/2512.deprecated.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
:func:`trio.move_on_after` and :func:`trio.fail_after` previously set the deadline relative to initialization time, instead of more intuitively upon entering the context manager. Any code where these two points in time do not line up (according to :func:`trio.current_time`) will now issue a `DeprecationWarning`. To silence the warning and adopt the new behaviour, pass ``timeout_from_enter = True``. For more information see :ref:`saved_relative_timeouts`.
1 change: 1 addition & 0 deletions newsfragments/2512.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
:func:`trio.move_on_after` and :func:`trio.fail_after` now allows setting the deadline to be relative to entering the context manager. This will be the default in the future, and to opt into the new behaviour you can pass ``timeout_from_enter=True``. This does not matter in the vast majority of cases where initialization and entering of the context manager is in the same place, and those cases will not raise any :class:`DeprecationWarning`. For more information see :ref:`saved_relative_timeouts`.
82 changes: 79 additions & 3 deletions src/trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from contextlib import AbstractAsyncContextManager, contextmanager, suppress
from contextvars import copy_context
from heapq import heapify, heappop, heappush
from math import inf
from math import inf, isnan
from time import perf_counter
from typing import (
TYPE_CHECKING,
Expand Down Expand Up @@ -543,8 +543,21 @@ class CancelScope:
cancelled_caught: bool = attrs.field(default=False, init=False)

# Constructor arguments:
_deadline: float = attrs.field(default=inf, kw_only=True, alias="deadline")
_shield: bool = attrs.field(default=False, kw_only=True, alias="shield")
_relative_deadline: float = attrs.field(default=inf, kw_only=True)
_deadline: float = attrs.field(default=inf, kw_only=True)
_shield: bool = attrs.field(default=False, kw_only=True)

def __attrs_post_init__(self) -> None:
if isnan(self._deadline):
raise ValueError("deadline must not be NaN")
if isnan(self._relative_deadline):
raise ValueError("relative deadline must not be NaN")
if self._relative_deadline < 0:
raise ValueError("timeout must be non-negative")
if self._relative_deadline != inf and self._deadline != inf:
raise ValueError(
"Cannot specify both a deadline and a relative deadline",
)

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

if self._relative_deadline != inf:
assert self._deadline == inf
self._deadline = current_time() + self._relative_deadline
self._relative_deadline = inf

if current_time() >= self._deadline:
self.cancel()
with self._might_change_registered_deadline():
Expand Down Expand Up @@ -734,13 +753,70 @@ def deadline(self) -> float:
this can be overridden by the ``deadline=`` argument to
the :class:`~trio.CancelScope` constructor.
"""
if self._relative_deadline != inf:
assert self._deadline == inf
warnings.warn(
DeprecationWarning(
"unentered relative cancel scope does not have an absolute deadline. Use `.relative_deadline`",
),
stacklevel=2,
)
return current_time() + self._relative_deadline
return self._deadline

@deadline.setter
def deadline(self, new_deadline: float) -> None:
if isnan(new_deadline):
raise ValueError("deadline must not be NaN")
if self._relative_deadline != inf:
assert self._deadline == inf
warnings.warn(
DeprecationWarning(
"unentered relative cancel scope does not have an absolute deadline. Transforming into an absolute cancel scope. First set `.relative_deadline = math.inf` if you do want an absolute cancel scope.",
),
stacklevel=2,
)
self._relative_deadline = inf
with self._might_change_registered_deadline():
self._deadline = float(new_deadline)

@property
def relative_deadline(self) -> float:
if self._has_been_entered:
return self._deadline - current_time()
elif self._deadline != inf:
assert self._relative_deadline == inf
raise RuntimeError(
"unentered non-relative cancel scope does not have a relative deadline",
)
return self._relative_deadline

@relative_deadline.setter
def relative_deadline(self, new_relative_deadline: float) -> None:
if isnan(new_relative_deadline):
raise ValueError("relative deadline must not be NaN")
if new_relative_deadline < 0:
raise ValueError("relative deadline must be non-negative")
if self._has_been_entered:
with self._might_change_registered_deadline():
self._deadline = current_time() + float(new_relative_deadline)
elif self._deadline != inf:
assert self._relative_deadline == inf
raise RuntimeError(
"unentered non-relative cancel scope does not have a relative deadline",
)
else:
self._relative_deadline = new_relative_deadline

@property
def is_relative(self) -> bool | None:
"""Returns None after entering. Returns False if both deadline and
relative_deadline are inf."""
assert not (self._deadline != inf and self._relative_deadline != inf)
if self._has_been_entered:
return None
return self._relative_deadline != inf

@property
def shield(self) -> bool:
"""Read-write, :class:`bool`, default :data:`False`. So long as
Expand Down
23 changes: 22 additions & 1 deletion src/trio/_core/_tests/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import types
import weakref
from contextlib import ExitStack, contextmanager, suppress
from math import inf
from math import inf, nan
from typing import TYPE_CHECKING, Any, NoReturn, TypeVar, cast

import outcome
Expand Down Expand Up @@ -365,6 +365,27 @@ async def test_cancel_scope_repr(mock_clock: _core.MockClock) -> None:
assert "exited" in repr(scope)


async def test_cancel_scope_validation() -> None:
with pytest.raises(
ValueError,
match="^Cannot specify both a deadline and a relative deadline$",
):
_core.CancelScope(deadline=7, relative_deadline=3)
scope = _core.CancelScope()

with pytest.raises(ValueError, match="^deadline must not be NaN$"):
scope.deadline = nan
with pytest.raises(ValueError, match="^relative deadline must not be NaN$"):
scope.relative_deadline = nan

with pytest.raises(ValueError, match="^relative deadline must be non-negative$"):
scope.relative_deadline = -3
scope.relative_deadline = 5
assert scope.relative_deadline == 5

# several related tests of CancelScope are implicitly handled by test_timeouts.py


def test_cancel_points() -> None:
async def main1() -> None:
with _core.CancelScope() as scope:
Expand Down
87 changes: 85 additions & 2 deletions src/trio/_tests/test_timeouts.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,20 @@
import outcome
import pytest

import trio

from .. import _core
from .._core._tests.tutil import slow
from .._timeouts import *
from .._timeouts import (
TooSlowError,
fail_after,
fail_at,
move_on_after,
move_on_at,
sleep,
sleep_forever,
sleep_until,
)
from ..testing import assert_checkpoints

T = TypeVar("T")
Expand Down Expand Up @@ -169,7 +180,79 @@
):
with pytest.raises(
ValueError,
match="^(duration|deadline|timeout) must (not )*be (non-negative|NaN)$",
match="^(duration|deadline|timeout|relative deadline) must (not )*be (non-negative|NaN)$",
):
Copy link
Member Author

@jakkdl jakkdl Sep 16, 2024

Choose a reason for hiding this comment

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

this is a mess. For non-relative timeouts the term is always "deadline", but for relative timeouts there are multiple terms used in different parts of the code:

Using relative_deadline everywhere probably isn't the best (await trio.sleep(relative_deadline=5)), but having four terms for essentially the same thing is very ugly.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd just update the error messages to refer to the argument name in all cases, using backticks to make it clear that this is an argument name if needed (e.g. maybe for "seconds"?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Since both move_on_after and CancelScope want to validate the same input parameter but currently have a different name for it, that would mean one of these has to be true:

  • move_on_after(seconds=-1) gives "relative_deadline" can't be negative
  • we duplicate validation code between move_on_after and CancelScope.__attrs_post_init__
  • move_on_after catches and reraises the exception from CancelScope solely to replace the parameter name in the error message.

Copy link
Member

Choose a reason for hiding this comment

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

Duplicating validation logic seems least bad of these options 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

There is another option we could do. Given how many names this parameter has, we could just add an additional parameter to CancelScope itself, with the name to use when reporting errors. Maybe that clutters the API too much though. But it does seem anyone else wrapping might also want to change the error reporting.

Copy link
Member Author

Choose a reason for hiding this comment

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

that sounds quite overcomplicated imo, esp since that would probably include both deadline and relative_deadline. If users are wrapping it and want pretty errors I don't think it's unreasonable to expect them to catch and reraise

with cm(val):
pass # pragma: no cover


async def test_timeout_deadline_on_entry(mock_clock: _core.MockClock) -> None:
rcs = move_on_after(5)
assert rcs.relative_deadline == 5

mock_clock.jump(3)
start = _core.current_time()
with rcs as cs:
assert cs.is_relative is None

# This would previously be start+2
assert cs.deadline == start + 5
assert cs.relative_deadline == 5

cs.deadline = start + 3
assert cs.deadline == start + 3
assert cs.relative_deadline == 3

cs.relative_deadline = 4
assert cs.deadline == start + 4
assert cs.relative_deadline == 4

rcs = move_on_after(5)
assert rcs.shield is False
rcs.shield = True
assert rcs.shield is True

mock_clock.jump(3)
start = _core.current_time()
with rcs as cs:
assert cs.deadline == start + 5

assert rcs is cs


async def test_invalid_access_unentered(mock_clock: _core.MockClock) -> None:
cs = move_on_after(5)
mock_clock.jump(3)
start = _core.current_time()

match_str = "^unentered relative cancel scope does not have an absolute deadline"
with pytest.warns(DeprecationWarning, match=match_str):
assert cs.deadline == start + 5
mock_clock.jump(1)
# this is hella sketchy, but they *have* been warned
with pytest.warns(DeprecationWarning, match=match_str):
assert cs.deadline == start + 6

with pytest.warns(DeprecationWarning, match=match_str):
cs.deadline = 7
# now transformed into absolute
assert cs.deadline == 7
assert not cs.is_relative

cs = move_on_at(5)

match_str = (
"^unentered non-relative cancel scope does not have a relative deadline$"
)
with pytest.raises(RuntimeError, match=match_str):
assert cs.relative_deadline
with pytest.raises(RuntimeError, match=match_str):
cs.relative_deadline = 7


@pytest.mark.xfail(reason="not implemented")
async def test_fail_access_before_entering() -> None:
my_fail_at = fail_at(5)
assert my_fail_at.deadline # type: ignore[attr-defined]
my_fail_after = fail_after(5)
assert my_fail_after.relative_deadline # type: ignore[attr-defined]

Check warning on line 258 in src/trio/_tests/test_timeouts.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_timeouts.py#L257-L258

Added lines #L257 - L258 were not covered by tests
59 changes: 40 additions & 19 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
import sys
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, *, shield: bool = False) -> trio.CancelScope:
"""Use as a context manager to create a cancel scope with the given
Expand All @@ -20,15 +23,19 @@
ValueError: if deadline is NaN.

"""
if math.isnan(deadline):
raise ValueError("deadline must not be NaN")
return trio.CancelScope(deadline=deadline, shield=shield)


def move_on_after(seconds: float, *, shield: bool = False) -> trio.CancelScope:
def move_on_after(
seconds: float,
*,
shield: bool = False,
) -> trio.CancelScope:
"""Use as a context manager to create a cancel scope whose deadline is
set to now + *seconds*.

The deadline of the cancel scope is calculated upon entering.

Args:
seconds (float): The timeout.
shield (bool): Initial value for the `~trio.CancelScope.shield` attribute
Expand All @@ -38,9 +45,10 @@
ValueError: if timeout is less than zero or NaN.

"""
if seconds < 0:
raise ValueError("timeout must be non-negative")
return move_on_at(trio.current_time() + seconds, shield=shield)
return trio.CancelScope(
shield=shield,
relative_deadline=seconds,
)


async def sleep_forever() -> None:
Expand Down Expand Up @@ -98,9 +106,12 @@
"""


# 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, *, shield: bool = False) -> AbstractContextManager[trio.CancelScope]: # type: ignore[misc]
@contextmanager
def fail_at(
deadline: float,
*,
shield: bool = False,
) -> Generator[trio.CancelScope, None, None]:
"""Creates a cancel scope with the given deadline, and raises an error if it
is actually cancelled.

Expand Down Expand Up @@ -129,15 +140,12 @@
raise TooSlowError


if not TYPE_CHECKING:
fail_at = contextmanager(fail_at)


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

Expand All @@ -148,6 +156,8 @@
it's caught and discarded. When it reaches :func:`fail_after`, then it's
caught and :exc:`TooSlowError` is raised in its place.

The deadline of the cancel scope is calculated upon entering.

Args:
seconds (float): The timeout.
shield (bool): Initial value for the `~trio.CancelScope.shield` attribute
Expand All @@ -159,6 +169,17 @@
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, shield=shield)
with move_on_after(seconds, shield=shield) as scope:
yield scope
if scope.cancelled_caught:
raise TooSlowError


# Users don't need to know that fail_at & fail_after wraps move_on_at and move_on_after
# and there is no functional difference. So we replace the return value when generating
# documentation.
if "sphinx" in sys.modules:
import inspect

Check warning on line 182 in src/trio/_timeouts.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_timeouts.py#L182

Added line #L182 was not covered by tests

for c in (fail_at, fail_after):
c.__signature__ = inspect.Signature.from_callable(c).replace(return_annotation=trio.CancelScope) # type: ignore[union-attr]

Check warning on line 185 in src/trio/_timeouts.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_timeouts.py#L185

Added line #L185 was not covered by tests
Loading