Can I get someone to have a look at Cancellation leaks out of asyncio.TaskGroup on 3.12 when using eager tasks · Issue #128588 · python/cpython · GitHub please?
This is resolved, probably the tests for wait_for/gather etc need to go through the same steps
I would argue that a lot of code actually depend on the subtle difference in the order in which tasks are run, because of the following construct:
tasks = set() # or any other thing that manages a collection of tasks
tasks.add(asyncio.create_task(coro()))
Before eager task factories, such code guarantees that when the body of coro runs, its task is in tasks already. When running eager task factories, the task won’t be in tasks until coro either blocks or becomes done (returned, raised, cancelled).
This construct is unlikely to go wrong if everything downstream of coro do not interact with tasks, but becomes dangerous if such interaction does happen, for example when coro also adds to tasks, and some cleanup logic somewhere cancels tasks in tasks.
Also, I don’t see an obvious correct way to fix this. I used this construct:
async def coro(tasks: set):
tasks.add(asyncio.current_task()) # for eager task factory
# actual work to be done
tasks = set()
new_task = asyncio.create_task(coro(tasks))
tasks.add(new_task) # for lazy task factory
But it only works because set.add is idempotent, and now I have to be very careful about removing from tasks, to prevent the case where the for lazy task factory line executes after coro has already completed and removed itself from tasks.
I’m tempted to say asyncio.Task needs a create_callback, just to handle such cases cleanly.
Does any core dev have an opinion on if gh-128307: support eager_start kwarg in create_eager_task_factory by graingert · Pull Request #128306 · python/cpython · GitHub is the desired API as a result of this thread and should be merged?
I have some quibbles with the PR but in general I believe the API is right – be able to override whether a task should be started eagerly at the create_task level seems the right granularity (as opposed to doing it on asyncio.run). Assuming @graingert responds to my review, once that’s settled it can go into beta 1 (next Tuesday, right?).
As someone who had recently to write a shield against eager task factory in my library (here, I am in favor of either removing eager tasks or having APIs to disallow them.
The issue with having options to use either is that it adds complexity to the API for both users and the libraries. In addition it makes it harder to implement custom loops.
In my view, eager tasks add a very small performance benefit for an added complexity that is not worthwhile. I believe much more significant performance gains could be had by optimizing task creation and the event loop.
For instance all asyncio’s objects (Handle, Task, etc) are Python classes. The even loop is Python based, etc. While this helps maintenance burden, this adds significant overhead. In addition as asyncio’s APIs check for subclasses, it’s not possible to implement faster variants (for instance having a Cython’s cdef class faster implementation).
This is related to the discussion of here where I get 4x to 20x faster work submission (depending on queue contention) on a threadpool with Cython cdef classes for Future and ThreadPool. I believe similar gains could be had with asyncio’s event loop.
EDIT: I incorrectly stated Task was a Python class. Indeed CPython has a C implementation for Task and Future. I still believe significant performance gains could be had, based on the gains observed for thread pools, and the performance of uvloop.
After spending several hours experimenting trying to implement a minimal event loop, analyzing where time was spent, and comparing to the default asyncio event loop (and its code), I must admit the code is really well optimized already.
Thus for the record, I retract my comment about further performance gains that could possibly be had.
The only potential optimization I could see, and for which I could measure small gains in my test event loop, would be to have fast paths for Future/Task’s call_soon calls when the event loop is the default one.