search

python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
5.98k stars 325 forks source link

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

Open jakkdl opened 3 weeks ago

jakkdl commented 3 weeks ago

Partially deals with #2512 by documenting current behavior.

This PR now works to fully resolve #2512 by adding a relative_deadline attribute to CancelScope, which is then used by fail_after and move_on_after. Upon entering the CancelScope the absolute deadline is then calculated as min(deadline, current_time() + relative_deadline).

This PR also reverts an old workaround in timeouts.py due to pycharm being unable to resolve @contextmanager for type checking.

TODO:

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.63%. Comparing base (80eec96) to head (480681d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3010 +/- ## ======================================= Coverage 99.63% 99.63% ======================================= Files 120 120 Lines 17865 17913 +48 Branches 3216 3226 +10 ======================================= + Hits 17800 17848 +48 Misses 46 46 Partials 19 19 ``` | [Files](https://app.codecov.io/gh/python-trio/trio/pull/3010?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) | Coverage Δ | | |---|---|---| | [src/trio/\_tests/test\_timeouts.py](https://app.codecov.io/gh/python-trio/trio/pull/3010?src=pr&el=tree&filepath=src%2Ftrio%2F_tests%2Ftest_timeouts.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3Rlc3RzL3Rlc3RfdGltZW91dHMucHk=) | `100.00% <100.00%> (ø)` | | | [src/trio/\_timeouts.py](https://app.codecov.io/gh/python-trio/trio/pull/3010?src=pr&el=tree&filepath=src%2Ftrio%2F_timeouts.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3RpbWVvdXRzLnB5) | `100.00% <100.00%> (ø)` | |
jakkdl commented 3 weeks ago

(ignore the commit history, I created the branch in a messy way. Will squash the PR anyway)

Zac-HD commented 3 weeks ago

Per my comment on the issue,

I think it would make sense for trio.fail_after(x) to be relative to entering the context manager; trio.fail_at(trio.current_time() + x) seems a more natural way to express the relative-to-construction-time semantics. (and the same for trio.move_on_{at,after})

Either way, we should document the chosen semantics.

While I think that documenting this is a net improvement, it might also lead people to rely on the creation-time semantics for *_after in a way that makes changing that to enter-time semantics harder later. Would you be up for making that change and documenting it as a one-step PR?

jakkdl commented 3 weeks ago

Per my comment on the issue,

I think it would make sense for trio.fail_after(x) to be relative to entering the context manager; trio.fail_at(trio.current_time() + x) seems a more natural way to express the relative-to-construction-time semantics. (and the same for trio.move_on_{at,after}) Either way, we should document the chosen semantics.

While I think that documenting this is a net improvement, it might also lead people to rely on the creation-time semantics for *_after in a way that makes changing that to enter-time semantics harder later. Would you be up for making that change and documenting it as a one-step PR?

Sure. Started thinking about different ways of implementing it in the issue: https://github.com/python-trio/trio/issues/2512#issuecomment-2157955978

jakkdl commented 1 week ago
  1. See https://github.com/python-trio/trio/issues/2512#issuecomment-2180329727 for discussion on the behavior of relative_deadline after entering.
  2. I personally prefer None over math.inf quite a bit, but changing the behavior of CancelScope.deadline would be breaking for anybody accessing the value of the property in the inf/None case - so I should probably just change relative_deadline to also use inf instead of None.
  3. Need to write docstring for relative_deadline, but can probably mostly just take the stuff I wrote in the newsfragment.
  4. Need to update docstrings for move_on_after and fail_after to be explicit about deadline being relative to entering. (i.e. undo/invert what was originally changed in this PR).

The pycharm bug referred to in timeouts seem to be fixed https://youtrack.jetbrains.com/issue/PY-36444/PyCharm-doesnt-infer-types-when-using-contextlib.contextmanager-decorator and released over a year ago, so I think we can drop the workaround... except mypy now behaves slightly different in one test (?).

jakkdl commented 1 week ago

RTD is failing because towncrier appends link to the github issue at the end of the newsfragments, but since we're inside a code block that leads to invalid python syntax. Trying to append newlines to the rst file doesn't help, they get stripped. Not sure the code blocks are really needed, could probably just move that stuff into the docstrings, but lets deal with that when we've decided on the precise implementation.

jakkdl commented 6 days ago

I gave it a shot to instead implement this as a transforming class. The basic implementation is way cleaner, but the big downside is that it will break code that currently does

cs = trio.move_on_after(5)
with cs:
  cs.deadline = 7  # AttributeError: _RelativeCancelScope does not have an attribute `deadline`
# you need to instead write
with cs as cs2:
  cs2.deadline = 7

There's a couple ways to fix that, though it's gonna be somewhat ugly. Either properties/methods that mirror the properties/methods of CancelScope, or dunder magic with __getattr__/__setattr__. We can make them invisible to type checkers and raise warnings though, and error if the user tries to access deadline before the scope is entered (if timeout_from_enter=True).