This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author asvetlov
Recipients ajoino, alex.gronholm, asvetlov, chris.jerdonek, gvanrossum, iritkatriel, jab, njs, tinchester, yselivanov
Date 2022-02-17.15:11:11
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1645110671.83.0.41667558618.issue46771@roundup.psfhosted.org>
In-reply-to
Content
The discussion is hot, I see several interleaved threads.

Let me put my answers on all of them in order of appearance.

1. quattro cancellation scopes are implemented after async-timeout.
As the author of async-timeout I am happy to know it.
The module is pretty small (as async-timeout itself).
I'd like to concentrate on the differences between async-timeout and quattro.

  a) quattro has `fail_after()` / `fail_at()` context managers,
  that are similar to async-timeout's `timeout()` / `timeout_at()`
  The first function schedules timeout as a *relative* delay,
  the second uses *absolut monotonic* time. So far so good.

  `timeout()` better points on 'what happens' I believe (TimeoutError is raised).
  `fail_after` is not that bad at all but `timeout()` is easier I think.
  `timeout_at()` and `fail_at()` are minic to `loop.call_at()`, the prefix is perfect.
  Regarding `loop.call_later()` I found that `timeout_later()` is too long name
  (plus `timeout()` context manager appeared in 2015 as a part of aiohttp).

  My opinion is biased, sure.

  b) quattro has `move_on_after()` and `move_on_at()` context managers that doesn't
  raise TimeoutError but set a flag after certain period.
  asyncio has lower level primitives than trio (`loop.call_later()` and `loop.call_at()`).
  Not sure that `move_on_*` should be added, they are low-level building blocks.
  On the other hand, `move_on_after()` requires less code lines than `call_later()`.
  If we decide 'yes' I suggest changing naming: I have no idea what is *moved* without
  reading the documentation.

  c) `with fail_after()` vs `async with timeout()`.
  The first async-timeout version used `with timeout()` notation.
  I've changed it to async counterpart after many questions from newbies:
  "Why is sync context manages used in async code?". Questions arose when asyncio was not
  wide-spread as today.
  People made their first baby steps those days, I provided a dozen of asyncio offline courses
  and many conference talks.
  Thus, please consider `async with ...` design as a user feedback reaction.
  As a side effect, it prohibits `timeout()` usage in non-async functions (which is
  awkward at least).
  Regarding async context manager performance, I think it is good enough
  for timeout-related things. I didn't experience any problem with it.
  Moreover, async fast-path (async function is called and it returns a value without
  suspension on awaiting) can be optimized on Python level to make it as fast as a regular
  python function call, sure. It is not trivial and might require adding a new opcode
  (combine CALL + GET_AWAITABLE) but this optimization is out of the issue scope.

  d) `cm.deadline += 0.5` vs `cm.shift(0.5)` is a question of taste.  asyncio-timeout
  design motivation was "don't do complex things in property setter" but I can live with
  mutable `cm.deadline` attribute, sure

  e) cancellation stack and `current_effective_deadline` -- I'm with Guido, let's not
  add yet another stack.  It can be an interesting debug feature but I doubt
  if it is useful in production code.
  Also, the performance cost is not zero. Merging and slicing stack tuple on any
  timeout context enter/exit is not free. The implementation can be switched
  to a linked list but still, do we really need it?

2. Alex Grönholm, asyncio supports custom task instances without overriding the entire
task factory. You should provide a custom method for custom task creatuon, that's it.
`asyncio.all_tasks()` / `asyncio.current_task()` support is provided by
'_register_task()', '_unregister_task()', '_enter_task()', and '_leave_task()' calls.
These methods are part of non-user-faced public API, they are intentionally enumerated
by `asyncio.__all__`.
These methods are mentioned by changelog only, sorry. A pull request for documenting them
in asyncio low-level section is welcome!

3. The race condition between two `.cancel()` calls performed by the same loop iteration.
Sure, the race exists.
Before TaskGroup landing, the last `.cancel()` wins. After the change, the first `.cancel()`
wins and all subsequent `.cancel()` calls made on the same event loop iteration are rejected
with returning `False`.  I believe, the changed behavior is more consistent (and close to
`Future.cancel()` design).

Assume, we have two scheduled close but different timeouts for the same tasks.
Both are reached at the next event loop iteration (see the timeline below):

prev-iteration   timeout-a   timeout-b  current-iteration
     |              |           |              |
 >---+--------------+-----------+--------------+--------------> future

I prepared https://github.com/aio-libs/async-timeout/pull/295 to handle it (not merged
yet because the next Python alpha release it required; I've tested it against the latest
CPython main branch manually). Sorry for polluting source code by `sys.version_info` checks,
the library is supposed to work with Python 3.7+.

async-timeout handles the race as follows:
https://github.com/aio-libs/async-timeout/blob/master/async_timeout/__init__.py#L209-L239

  a) timeout-a and timeout-b TimeHandle's are scheduled for execution
  on the current iteration and executed.
  b) Both context managers set their own state to 'TIMEOUT'.
  c) timeout-a cancels the current task, timeout-b calls the cancel but it is
  skipped and rejected because timeout-a called `task.cancel()` earlier.
  d) The task is cancelled, async stack unwinds.
  e) The inner context manager converts CancelledError to TimeoutError and raises it.
  f) The outer context manager does nothing but bubbles-up TimeoutError from
  the inner context manager.

Doesn't matter what context manager (timeout-a or timeout-b) is inner and what is outer;
it the timeout occurs the TimeoutError is propagated from the inner code up along the stack
*unless* some code swallows it intentionally.
I don't think that we should prohibit swallowing.

Another edge case is the task explicit cancellation that is scheduled on the same event
loop iteration as timeout occurrence:

prev-iteration (explicit .cancel() called)   timeout  current-iteration
		   |                            |             |
 >-----------------+----------------------------+-------------+----------> future


  a) `loop.call_later()` registered callback (`_on_timeout`) is called first.
  Its `task.cancel()` is rejected (`False` returned) because the first cancellation
  was explicitly requested on the previous iteration.
  b) the task wakes up to handle `.cancel()` call from the prev iteration
  c) stack unwinds with CancelledError, it is not converted to TimeoutError because
  CancelledError has no required message. That's fine, because `.cancel()` call from
  the previous iteration is processed, not timeout itself.
  d) `.expired` property is `True` though because timeout is reached technically;
  the wall-clock time is greater than the deadline.


4. Tin Tvrtković, I don't think that a separate field for 'nonce' is needed for the
proper cancellation.

It adds more complexity; I have no idea what to do with the nonce field if the
CancelledError comes not from timeout context manager but is rased by other logic.

As I demonstrated above, using cancellation message as nonce work perfectly fine.


P.S. It is a long letter, sorry. Please don't hesitate to discuss it, feel free to ask a
question if some of my words are not clear. English is not my mother language :(
History
Date User Action Args
2022-02-17 15:11:11asvetlovsetrecipients: + asvetlov, gvanrossum, njs, jab, alex.gronholm, chris.jerdonek, yselivanov, tinchester, iritkatriel, ajoino
2022-02-17 15:11:11asvetlovsetmessageid: <1645110671.83.0.41667558618.issue46771@roundup.psfhosted.org>
2022-02-17 15:11:11asvetlovlinkissue46771 messages
2022-02-17 15:11:11asvetlovcreate