Merge pull request #1265 from JoshuaWatt/ssh-deadlock-fix

ssh: Prevent timeout from deadlock

labgrid/driver/sshdriver.py

@@ -136,9 +136,8 @@ def _start_own_master_once(self, timeout):
 
         try:
             subprocess_timeout = timeout + 5
-            return_value = self.process.wait(timeout=subprocess_timeout)
-            if return_value != 0:
-                stdout, _ = self.process.communicate(timeout=subprocess_timeout)
+            stdout, _ = self.process.communicate(timeout=subprocess_timeout)
+            if self.process.returncode != 0:
                 stdout = stdout.split(b"\n")
                 for line in stdout:
                     self.logger.warning("ssh: %s", line.rstrip().decode(encoding="utf-8", errors="replace"))
@@ -155,12 +154,14 @@ def _start_own_master_once(self, timeout):
                     pass
 
                 raise ExecutionError(
-                    f"Failed to connect to {self.networkservice.address} with {' '.join(args)}: return code {return_value}",  # pylint: disable=line-too-long
+                    f"Failed to connect to {self.networkservice.address} with {' '.join(args)}: return code {self.process.returncode}",  # pylint: disable=line-too-long
                     stdout=stdout,
                 )
         except subprocess.TimeoutExpired:
+            self.process.kill()
+            stdout, _ = self.process.communicate()
             raise ExecutionError(
-                f"Subprocess timed out [{subprocess_timeout}s] while executing {args}",
+                f"Subprocess timed out [{subprocess_timeout}s] while executing {args}: {stdout}",
             )
         finally:
             if self.networkservice.password and os.path.exists(pass_file):

labgrid/util/ssh.py

@@ -453,8 +453,8 @@ def _start_own_master(self):
         )
 
         try:
-            if self._master.wait(timeout=connect_timeout) != 0:
-                stdout, stderr = self._master.communicate()
+            stdout, stderr = self._master.communicate(timeout=connect_timeout)
+            if self._master.returncode != 0:
                 raise ExecutionError(
                     f"failed to connect to {self.host} with args {args}, returncode={self._master.returncode} {stdout},{stderr}"  # pylint: disable=line-too-long
                 )

tests/test_sshdriver.py

@@ -17,6 +17,8 @@ def ssh_driver_mocked_and_activated(target, mocker):
     instance_mock = mocker.MagicMock()
     popen.return_value = instance_mock
     instance_mock.wait = mocker.MagicMock(return_value=0)
+    instance_mock.communicate = mocker.MagicMock(return_value=(b"", b""))
+    instance_mock.returncode = 0
     SSHDriver(target, "ssh")
     s = target.get_driver("SSHDriver")
     return s
@@ -35,6 +37,8 @@ def test_create(target, mocker):
     instance_mock = mocker.MagicMock()
     popen.return_value = instance_mock
     instance_mock.wait = mocker.MagicMock(return_value=0)
+    instance_mock.communicate = mocker.MagicMock(return_value=(b"", b""))
+    instance_mock.returncode = 0
     s = SSHDriver(target, "ssh")
     assert isinstance(s, SSHDriver)