Skip to content
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.

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

Store references to tasks #718

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Birnendampf
Copy link

closes #716
As discussed, SSHConnection now has a _tasks attribute. Created tasks are stored in it and removed when they are done.
When wait_closed is called, all running tasks are awaited.
What i am unsure about: should aborting the connection cancel all running tasks? The idea here is that there needs to be a way to stop tasks that never finish. The user can try to close the connection gracefully, wait for it to close with a timeout and abort the connection if the timeout is exceeded.
The abort() method itself seems to be the right place to cancel all tasks, as _force_close() and _cleanup() are also called in a normal close/disconnect operation, so cancelling all tasks there would make awaiting them pointless

@Birnendampf Birnendampf marked this pull request as ready for review November 21, 2024 21:36
…hould this be intended? it would mean putting a timeout on wait_closed would also cancel the tasks if it runs out, which seems counterintuitive
@ronf
Copy link
Owner

ronf commented Nov 22, 2024

Keep in mind that wait_closed() can be called even when the connection is still fully open and in the middle of doing things that might take arbitrarily long to complete. The idea here is that something else in the application would automatically close the connection at some point, and that would cause wait_closed() to unblock. So, while an application is free to wrap a timeout around wait_closed() or otherwise decide to explicitly abort a connection on something like a timer, I don't think SSHConnection should ever do such a thing automatically.

Related to that, the cleanup functions in AsyncSSH are already designed to unblock everything related to network I/O on a closed or aborted connection. There's also already code to automatically cancel any auth in progress, as well as canceling the login and keepalive timers.

So, I'm wondering if the best option in wait_closed() might be to leave that as-is. After all, it might not even be called, so there's no guarantee the tasks will finish before the SSHConnection is abandoned. Similarly, we can't count on abort() being called (in fact, it's relatively uncommon). The common function to do this in would be _cleanup(), but I think it's generally better to let tasks finish cleanly rather than cancelling them. If they hang around long enough for the garbage collector to cancel them after the strong reference in SSHConnection has gone away, that actually seems like it might be the right thing.

Another factor here is that _cleanup() and related functions like _force_close() are not async, so they can't actually wait on anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSHConnection does not save a reference to tasks it creates
2 participants