SSHConnection does not save a reference to tasks it creates

[Birnendampf]

This function: https://github.com/ronf/asyncssh/blob/ca15e1123ff335feea5a74926feb97a8f04986cb/asyncssh/connection.py#L1097-L1104 is used to schedule coroutines from within synchronous functions. It returns the task it creates, but that return value is often ignored, which is a problem because the event loop only keeps weak references to tasks (the same goes for tasks created using asyncio.ensure_future()). Unless I am missing something, this means that tasks created using SSHConnection.create_task() might be garbage collected and disappear at any time. The fact this has not yet caused any problems for anybody could just be because of some CPython implementation detail which asyncssh should not rely on.

Proposed fix

• add a _tasks attribute to SSHConnections which is initialized to be an empty set
• SSHConnection.create_task() adds the task it creates to the set
• SSHConnection._reap_task() removes the task from the set

Even if the problem described above seems to never have occurred, the fix is really simple and should not impact performance, so maybe it should be done anyways just to be sure.

[ronf]

Thanks for the report, and the pointer to the documentation about the weak references. I'm rather surprised that no issues with this have been seen if that's truly the case. That said, I totally agree with you that this may be unsafe.

Adding an explicit list of tasks has another potential benefit: It would allow the SSH connection object to potentially wait for all the tasks associated with the connection to be reaped once the connection is closed (blocking on this in wait_closed). This might prevent the event loop from exiting with some tasks still running, which I have occasionally seen happen.

I'm in the middle of some other changes right now, but I'll look into this further once that's complete.

[Birnendampf]

Thanks for the quick response! This is actually a somewhat infamous issue/design flaw of asyncio tasks and the reason many people don't know about it is because the warning was only added in the documentation for python 3.9 and because it is a pretty niche edge case. If you are curious when exactly this happens, this user on stack overflow wrote a pretty detailed explanation: https://stackoverflow.com/questions/71938799/python-asyncio-create-task-really-need-to-keep-a-reference/76823668#76823668

[ronf]

This note appearing only in Python 3.9 definitely explains why I hadn't seen it. I'm pretty sure the create_task() and _reap_task() on SSHConnection is much longer than Python 3.9 and hasn't changed much since it was originally written.

Thanks also for the pointer to the longer explanation. I'm not sure I followed all of the details there, but it was an interesting analysis.

Keeping references in the SSHConnection object to all the tasks created by the connection's create_task() and removing them in _reap_task() seems like it should work as long as a strong reference is maintained to the SSHConnection objects themselves (to avoid having only a circular dependency). That should be fine, though, as AsyncSSH application code is generally going to have strong external references to the connection objects, at least until a connection is closed.