fix(connection): wrap reconnect in a task

[gboutry]

Description

Wrap reconnect coroutine in a task. This is due to a breaking change in python stdlib 3.11. Before 3.11 it only emitted a warning.

Fixes: #1102

All places calling asyncio.wait are already wrapping into a task:

python-libjuju/juju/model.py

Line 757 in 7d2107f

done, pending = await jasyncio.wait([waiter, self._watcher_task],

python-libjuju/juju/utils.py

Line 179 in 7d2107f

done, pending = await jasyncio.wait([task] + event_tasks,

[jujubot]

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

[jujubot]

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

[james-garner-canonical]

/build

[james-garner-canonical]

I'm not an expert here but this looks good to me. As noted, all the other calls to wait already use tasks. Likewise, all the other calls to reconnect are wrapped (in ensure_future), so this seems like an appropriate solution here.

[benhoyt]

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?

[james-garner-canonical]

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

[gboutry]

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.

[dimaqq]

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.

[dimaqq]

My 2c:

cancellation safety is a noble goal, but it won't be achieved by shielding this one task alone.

I would propose to keep this PR simple: only wrap coro in a task, which I believe, if a non-breaking change, as in it preserves current behaviour, good or bad.

Let's open a separate issue about cancellation.

[dimaqq]

Either fix will move us forward.

[dimaqq]

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.

[gboutry]

I would rather keep the current fix. It'll be the same as the 2 others places this is called with just the goal of unblocking our functional tests on Noble.

An other PR can come later reworking this more broadly in the library.

[james-garner-canonical]

/build

[benhoyt]

/merge