Improve handling of exceptions in connection_lost() callbacks

This commit adds wrappers around calls to connection_lost() in the
SSHClient, SSHServer, SSHClientSession, and SSHServerSession classes
to make sure that cleanup completes properly in the SSHConnection and
SSHChannel classes. Exceptions in connection_lost() will be reported
in the debug log but cleanup operations will continue, ignoring those
exceptions. Thanks go to Danil Slinchuk for reporting this issue!

Author: ronf

asyncssh/channel.py

@@ -26,6 +26,7 @@
 import inspect
 import re
 import signal as _signal
+import sys
 from types import MappingProxyType
 from typing import TYPE_CHECKING, Any, AnyStr, Awaitable, Callable
 from typing import Dict, Generic, Iterable, List, Mapping, Optional
@@ -225,7 +226,13 @@ def _cleanup(self, exc: Optional[Exception] = None) -> None:
             self._request_waiters = []
 
         if self._session is not None:
-            self._session.connection_lost(exc)
+            # pylint: disable=broad-except
+            try:
+                self._session.connection_lost(exc)
+            except Exception:
+                self.logger.debug1('Uncaught exception in session ignored',
+                                   exc_info=sys.exc_info)
+
             self._session = None
 
         self._close_event.set()

asyncssh/connection.py

@@ -1075,7 +1075,13 @@ def _cleanup(self, exc: Optional[Exception]) -> None:
             self._wait = None
 
         if self._owner: # pragma: no branch
-            self._owner.connection_lost(exc)
+            # pylint: disable=broad-except
+            try:
+                self._owner.connection_lost(exc)
+            except Exception:
+                self.logger.debug1('Uncaught exception in owner ignored',
+                                   exc_info=sys.exc_info)
+
             self._owner = None
 
         self._cancel_login_timer()

tests/test_channel.py

@@ -247,6 +247,15 @@ def session_started(self):
         chan.close()
 
 
+class _ClientSessionCleanupError(asyncssh.SSHClientSession):
+    """Test of exception during client session cleanup"""
+
+    def connection_lost(self, exc):
+        """Raise an error when a client session is cleaned up"""
+
+        raise RuntimeError('Exception in session cleanup test')
+
+
 class _ChannelServer(Server):
     """Server for testing the AsyncSSH channel API"""
 
@@ -1761,6 +1770,13 @@ async def test_unknown_action(self):
             await chan.wait_closed()
             self.assertEqual(session.exit_status, 255)
 
+    @asynctest
+    async def test_client_session_cleanup_error(self):
+        """Test error in client session cleanup"""
+
+        async with self.connect() as conn:
+            await conn.create_session(_ClientSessionCleanupError)
+
 
 class _TestChannelNoPTY(ServerTestCase):
     """Unit tests for AsyncSSH channel module with PTYs disallowed"""

tests/test_connection.py

@@ -282,6 +282,17 @@ def connection_made(self, conn):
         raise RuntimeError('Exception handler test')
 
 
+class _ClientCleanupError(asyncssh.SSHClient):
+    """Test of exception during client cleanup"""
+
+    def connection_lost(self, exc):
+        """Raise an error when a client is cleaned up"""
+
+        # pylint: disable=unused-argument
+
+        raise RuntimeError('Exception in cleanup test')
+
+
 class _TunnelServer(Server):
     """Allow forwarding to test server host key request tunneling"""
 
@@ -1719,6 +1730,13 @@ async def test_internal_error(self):
         with self.assertRaises(RuntimeError):
             await self.create_connection(_InternalErrorClient)
 
+    @asynctest
+    async def test_client_cleanup_error(self):
+        """Test error in client cleanup"""
+
+        async with self.connect(client_factory=_ClientCleanupError):
+            pass
+
 
 @patch_extra_kex
 class _TestConnectionNoStrictKex(ServerTestCase):