Closed maximilianriemensberger closed 3 years ago
I had a quick look why the test fails. I guess we run into an infinite recursion calling cancel_all_tasks() if connection is lost. It starts in the IncompleteReadError exception in read_data_loop. Maybe the following would fix this?
async def cancel_all_tasks() -> None:
"""Cancel all pypck tasks."""
while PYPCK_TASKS:
await cancel_task(PYPCK_TASKS.pop())
Thanks. I'll take a look.
@alengwenus Sorry for huge delay in looking into this PR.
I turns out that close path is pretty intricate. This version passes the tests. However, the seemingly small change pop(0)
(cancel oldest task first) to pop(-1)
(cancel most recent task first) beaks the connection lost test.
On the connection lost path we have the following steps:
IncompleteReadError
raisedcreate_task(self.event_handler("connection-lost"))
starts the event_handler and registers the task in PYPCK_TASKS
await self.async_close()
calls await cancel_tasks()
By the way this problem is not specific to the changes in the PR. It the current code were to iterate through the tuple in reverse order it would run into the same problem.
How to solve it? I'm not sure yet. So far I have considered multiple approaches:
I guess there are probably further options available. Right now I would probably stick with the first option (Do nothing). But if you have a different preference let me know.
@maximilianriemensberger I completely fogot about that, too. Great that you are coming back to it. Thanks a lot! Now, I have to get my bearings again.
To be honest, removing tasks in the reverse order and relying on a proper task order sound a bit fiddly to me (although it might work in this case).
My intention to implement the event handler was to notify the program which uses pypck
about what's going on.
Having this in mind, I would suggest to not wrap the event_handler
call into a task but simply await
it (maybe only within the IncompleteReadError
exception).
Then, the calling program could create tasks on its own and if necessary store them in an own task registry.
@alengwenus The order of cancellation in this PR is currently the same as it is on the current master branch. So I did not change it here. One reason for not changing the order is that it would break the event_handler.
Regarding the event_handler, I'll open a separate PR with await event_handler
it instead of create_task(event_handler)
. If no current user is doing any long running async tasks, this should be fine. Do you know of any users (like home assistant) that does significant work in the event_handler
?
Another thing that came to my mind while thinking about cacellation: The task registry is right now process global. This means if you are running multiple PchkConnections in your process, cancel_all_tasks
as part of an async_close
in one connection will cancel all tasks of all connections. This is probably not how it is intended. But I'm not sure how common this use case is. Would it affect home assistant when it has multiple LCN connection configured? Is that actually possible? I'm considering to address this issue as well unless you prefer to keep it as is or address it your own. In my opinion one way of dealing with it is to pin the task registry (current PYPCK_TASKS
) to the connection or connection manager.
Regarding the event_handler, I'll open a separate PR with await event_handler it instead of create_task(event_handler). If no current user is doing any long running async tasks, this should be fine. Do you know of any users (like home assistant) that does significant work in the event_handler?
I just saw that you have a related change on a branch already. So I'll assume you just take this one.
@maximilianriemensberger The global task registry indeed can be a problem when dealing with multiple connections. Indeed it is possible to add multiple connections within HomeAssistant. So making the task registry a member of PchkConnection should be the way to go.
Regarding the event_handler, I'll open a separate PR with await event_handler it instead of create_task(event_handler). If no current user is doing any long running async tasks, this should be fine. Do you know of any users (like home assistant) that does significant work in the event_handler?
I just saw that you have a related change on a branch already. So I'll assume you just take this one.
Done: #79
@alengwenus The order of cancellation in this PR is currently the same as it is on the current master branch. So I did not change it here. One reason for not changing the order is that it would break the event_handler.
Regarding the event_handler, I'll open a separate PR with
await event_handler
it instead ofcreate_task(event_handler)
. If no current user is doing any long running async tasks, this should be fine. Do you know of any users (like home assistant) that does significant work in theevent_handler
?Another thing that came to my mind while thinking about cacellation: The task registry is right now process global. This means if you are running multiple PchkConnections in your process,
cancel_all_tasks
as part of anasync_close
in one connection will cancel all tasks of all connections. This is probably not how it is intended. But I'm not sure how common this use case is. Would it affect home assistant when it has multiple LCN connection configured? Is that actually possible? I'm considering to address this issue as well unless you prefer to keep it as is or address it your own. In my opinion one way of dealing with it is to pin the task registry (currentPYPCK_TASKS
) to the connection or connection manager.
I opened an issue for this: #80 @maximilianriemensberger Are you going to take it over?
@alengwenus This implements what we discussed in #73