Skip to content
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

bump min python to 3.8, and migrate to aiorpcx 0.22 #7661

Merged
merged 7 commits into from
Feb 16, 2022

Conversation

SomberNight
Copy link
Member

This PR

Re the aiorpcx changes, note that the external API of that library changed.

  • Most notable is that when using a TaskGroup in an async with statement, exceptions encountered by enclosed tasks are not re-raised in __aexit__. Instead, it is now the caller's responsibility to inspect the results and exceptions of the tasks.
    For old code that looks like this:

    async with TaskGroup() as group:
      await group.spawn(task1())
      await group.spawn(task2())

    kyuupichan and upstream curio recommends to write instead (for new code):

    async with TaskGroup() as group:
      await group.spawn(task1())
      await group.spawn(task2())
      
      async for task in group:
        if not task.cancelled():
          task.result()

    Instead, in this PR I introduce class OldTaskGroup(aiorpcx.TaskGroup), which retains the old behaviour of TaskGroup.
    see TaskGroup(wait=all), weird behaviour and API of completed kyuupichan/aiorpcX#43

  • Also note that there is a timing issue/race with the timeout_after and ignore_after APIs of aiorpcx. I think the issue has been there for a long time - the changes in TaskGroup simply exposed it.
    Consider example:

    async def outer_task():
      async with timeout_after(0.1):
        await inner_task()

    When the 0.1 sec timeout expires, inner_task will get cancelled by timeout_after (=internal cancellation).
    If around the same time (in terms of event loop iterations) another coroutine cancels outer_task (=external cancellation), there will be a race.
    Both cancellations work by propagating a CancelledError out to timeout_after, which then
    needs to decide (in TimeoutAfter.__aexit__) whether it's due to an internal or external cancellation.
    AFAICT asyncio provides no reliable way of distinguishing between the two.
    This patch tries to always give priority to external cancellations.
    see ignore_after cannot be cancelled sometimes (race?) kyuupichan/aiorpcX#44
    see aiohttp swallows asyncio.CancelledError during connection timeout aio-libs/async-timeout#229
    see https://bugs.python.org/issue42130 and https://bugs.python.org/issue45098

    I believe the problem became more relevant with new aiorpcx due to changes to TaskGroup.join() (which is called in __aexit__) and TaskGroup.cancel_remaining() in particular. When the group encounters an exception in a task, all other tasks are cancelled. Old aiorpcx used to not wait for the cancellations to finish, but new aiorpcx blocks until the cancellations are done. If the timing race occurs, a cancellation gets suppressed, and cancel_remaining() blocks indefinitely.
    To fix this, this PR is monkey-patching aiorpcx to include a workaround for the race.

In particular, asyncio.CancelledError no longer inherits Exception (it inherits BaseException directly)
I think this was originally needed due to incorrect management of group lifecycles,
which our current code is doing better.

also note that if we needed this, in newer aiorpcx, the name of
the field was ~changed from `_closed` to `joined`:
kyuupichan/aiorpcX@2390026
aiorpcx 0.20 changed the behaviour/API of TaskGroups.
When used as a context manager, TaskGroups no longer propagate
exceptions raised by their tasks. Instead, the calling code has
to explicitly check the results of tasks and decide whether to re-raise
any exceptions.
This is a significant change, and so this commit introduces "OldTaskGroup",
which should behave as the TaskGroup class of old aiorpcx. All existing
usages of TaskGroup are replaced with OldTaskGroup.

closes spesmilo#7446
@SomberNight SomberNight merged commit 586d3a4 into spesmilo:master Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bump required python version to 3.8 aiorpcX 0.22 support
1 participant