check for tasks before cancelling them #533
Conversation
| try: | ||
| result = state is None and client.uaclient.protocol is None or \ | ||
| client.uaclient.protocol.state is state | ||
| if result: | ||
| break | ||
| except AttributeError: # protocol doesn't exist yet (connection attempt ongoing), but target is not None | ||
| pass | ||
| await sleep(SLEEP) | ||
| assert client.uaclient.protocol.state == state | ||
| assert result | ||
|
|
There was a problem hiding this comment.
Why not changing just this bit?
if (
client.uaclient.protocol == state or
client.uaclient.protocol and client.uaclient.protocol.state == state
):
nit: is compares object and == compares values, better being explicit here.
There was a problem hiding this comment.
The method gives the test several tries before asserting a False, so that if after 3 seconds the test comes out positive it'll return immediately instead of waiting the full timeout. In order to not evaluate the expression twice (also DRY) I used a storage variable.
The reason for is instead of == is that the arguments in the function are enums, so either UaProtocol.INITIALIZED/OPEN/CLOSED or None. IMO any other value would be wrong and shouldn't be accepted.
| except asyncio.CancelledError: | ||
| pass | ||
| finally: | ||
| self._connect_task = None |
There was a problem hiding this comment.
If we end up here, we have a connect coroutine running. What's the use-case where we need to set self._connect_task = None again?
There was a problem hiding this comment.
If we end up here, a connect task is running and being awaited inside the try block just above. Another task doing something else might call disconnect and cancel the connect task, resulting in a CancelledError. In any case, the _connect_task is either cancelled or done after the await inside the try block, which is why resetting it to None signals that it's not running.
It's not explicitly necessary, but returning to initial state means you only have to worry about "there's a task and it's running or there's no task" and not check if the task is done or cancelled or whatever when disconnecting.
| except asyncio.CancelledError: | ||
| pass | ||
| finally: | ||
| self._connect_task = None |
There was a problem hiding this comment.
why putting the connection in the background then waiting for it?? It does not make any sense
There was a problem hiding this comment.
So that disconnect can cancel the task. The connect behavior is supposed to be blocking (to preserve the behavior it has so far), but disconnect should be able to cancel the process. Therefore, a task is needed to be cancellable and awaiting it keeps the process blocking.
There was a problem hiding this comment.
in theory in a normal applicaiton you call await connect()then await disconnect() An application calling disconnect() while doing calling connect() is doing something quite unusual and should handle its own shit itself. They will anyway do a much better job than we do at low level
There was a problem hiding this comment.
¯\_(ツ)_/¯ the use case was given by ZuZuD. Without it, checking for an existing protocol before trying to disonnect it would be sufficient.
There was a problem hiding this comment.
I never looked at his code. But for sure he should have his own tasks and can cancel them. It does need even more tasks inside his tasks
There was a problem hiding this comment.
I don't follow. It was the first code review for this PR and the reason why a simple check for client.protocol was insufficient.
If you don't think it's necessary I'll revert it.
There was a problem hiding this comment.
in theory in a normal applicaiton you call
await connect()thenawait disconnect()An application calling disconnect() while doing calling connect() is doing something quite unusual and should handle its own shit itself. They will anyway do a much better job than we do at low level
I can't agree more with this and that's why we chose to deal with this at the HA level.
There was a problem hiding this comment.
@Randelung sorry I did not follow and do not have the capacity to folow everything. So I just check PR when I got 5 minutes and comment what I see. I do not know the history but clearly a wrapper making task of a method and just awaiting it ooks very strnage and you must have a very clear use case for such a thing. It does not seem to fit the current API
There was a problem hiding this comment.
Yeah, no worries, I know how it is.
How do we continue? I think at this point it's more of a design choice.
Fixes #337.
Have a look at the changes and see if you want the
Nonetasks to not trigger a log warning or not.