Skip to content

Use task.uncancel when suppressing cancellation in ConnectionPool #8108

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.

Sign up for GitHub

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

Closed
fjetter wants to merge 1 commit into from
Closed

Use task.uncancel when suppressing cancellation in ConnectionPool #8108

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions distributed/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,6 +1544,8 @@ async def _connect(self, addr: str, timeout: float | None = None) -> Comm:
self._connecting_count -= 1
except asyncio.CancelledError:
current_task = asyncio.current_task()
if sys.version_info >= (3, 11):
current_task.uncancel()

@graingert graingert Aug 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't quite the right usage for uncancel. It should only be called if self._reasons.pop(current_task is set and then CancelledError should be re-raised if uncancel() != 0

@graingert graingert Aug 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One option is to (on python3.11) use

try:
    async asyncio.timeout(math.inf) as scope:
        await ...
except TimeoutError:
    raise CommClosedError(...)

And then scope.reschedule(-1) to cancel the task, then we can rely on cpython code to catch the CancelledError and call uncancel() correctly

@fjetter fjetter Aug 15, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, but I won't rewrite the entire code for a py3.11 only option

@graingert graingert Aug 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I already started on it here: #8109 😝

@graingert graingert Aug 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's worth it as we can skip a load of bookkeeping for external/internal cancellation on 3.11, and "end-user code" isn't really supposed to call .uncancel():

This method is used by asyncio’s internals and isn’t expected to be used by end-user code.

@graingert graingert Aug 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also I was wrong on how to use uncancel() correctly here, You need to record the cancelling state: https://github.com/python/cpython/blob/e2420c5cae7a173c5242b3507979010a933e53a7/Lib/asyncio/timeouts.py#L87 then check https://github.com/python/cpython/blob/3.11/Lib/asyncio/timeouts.py#L108 if self._task.uncancel() <= self._cancelling and exc_type is exceptions.CancelledError:

assert current_task
reason = self._reasons.pop(current_task, "ConnectionPool closing.")
raise CommClosedError(reason)
Expand Down