Skip to content

fix(connection): wrap reconnect in a task #1103

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

Merged
jujubot merged 1 commit into from
Sep 26, 2024
Merged

fix(connection): wrap reconnect in a task #1103

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: 1 addition & 1 deletion juju/client/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ async def rpc(self, msg, encoder=None):
# if it is triggered by the pinger, then this RPC call will
# be cancelled when the pinger is cancelled by the reconnect,
# and we don't want the reconnect to be aborted halfway through
await jasyncio.wait([self.reconnect()])
await jasyncio.wait([jasyncio.create_task(self.reconnect())])
Copy link
Copy Markdown
Collaborator

@benhoyt benhoyt Sep 24, 2024

Choose a reason for hiding this comment

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

How is this different from just await self.reconnect()? It looks like asyncio.wait() is supposed to be used when you're waiting for multiple concurrent tasks. I'm not at all up on asyncio -- maybe @dimaqq could chime in too?

Copy link
Copy Markdown
Contributor

@james-garner-canonical james-garner-canonical Sep 24, 2024

Choose a reason for hiding this comment

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

True, I just assumed that asyncio was being used for a good reason, but I'm not super familiar with it either. Perhaps related to the comment above this line?

# the reconnect has to be done in a separate task because,
# if it is triggered by the pinger, then this RPC call will
# be cancelled when the pinger is cancelled by the reconnect,
# and we don't want the reconnect to be aborted halfway through

Copy link
Copy Markdown
Contributor Author

@gboutry gboutry Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This comment and call have been introduced in https://github.com/juju/python-libjuju/pull/148/files.

From the comment, we can infer that the caller wants the coroutine reconnect to be continue even if the rpc method is itself cancelled.

One of the side effect of the previous implementation of asyncio.wait was shielding the coroutine from the caller cancellation.

To make this behaviour clearer, we could instead:

await jasyncio.shield(jasyncio.create_task(self.reconnect()))

The shield would still yield a CancelledError, but the task inside would continue in the background.

Copy link
Copy Markdown
Contributor

@dimaqq dimaqq Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Given that there's a single item in the wait list, wdyt about this instead?

Suggested change
await jasyncio.wait([jasyncio.create_task(self.reconnect())])
await self.reconnect()

Maybe with a touch-up of the comment above, clarifying that cancellation mid-reconnect is an outstanding issue.

if self.monitor.status != Monitor.CONNECTED:
# reconnect failed; abort and shutdown
log.error('RPC: Automatic reconnect failed')
Expand Down