fix: Handle stderr properly in SSH transport

Instead of reading a small amount of data in a blocking way from the
stdout and stderr streams, enable non-blocking I/O and attempt to read
as much data as possible. In addition, make a convenience function to
handle the stream being closed and timeouts so that we don't need to
store a temp variable in the loop. This prevents the situation that lead
to issue #74 where the stderr data is written to the output XML if
stdout is already closed.

Fixes #74

Author: kadler

src/itoolkit/transport/ssh.py

@@ -23,6 +23,8 @@
 """
 from .base import XmlServiceTransport
 
+import socket
+
 __all__ = [
     'SshTransport'
 ]
@@ -75,33 +77,45 @@ def _call(self, tk):
         """
         command = "/QOpenSys/pkgs/bin/xmlservice-cli"
         stdin, stdout, stderr = self.conn.exec_command(command)
+        channel = stdout.channel
+
         xml_in = tk.xml_in()
         stdin.write(xml_in.encode())
-        stdin.flush()
-        stdin.channel.shutdown_write()
+        stdin.close()
+        channel.shutdown_write()
+
+        # Disable blocking I/O
+        # chan.settimeout(0.0) is equivalent to chan.setblocking(0)
+        # https://docs.paramiko.org/en/stable/api/channel.html#paramiko.channel.Channel.settimeout
+        channel.settimeout(0.0)
 
         # rather than doing all this loop-de-loop, we could instead just use
         # a single call to stdout.readlines(), but there is a remote
         # possibility that the process is hanging writing to a filled-up
-        # stderr pipe. So, we read a little from both until we're all done
+        # stderr pipe. So, we read from both until we're all done
         err_out = b""
         xml_out = b""
-        blockSize = 64  # arbitrary
-        while not stderr.closed or not stdout.closed:
-            if not stderr.closed:
-                newData = stderr.read(blockSize)
-            if not newData:
-                stderr.close()  # reaching EOF doesn't implicitly close
-            else:
-                err_out += newData
-            if not stdout.closed:
-                newData = stdout.read(blockSize)
-            if not newData:
-                stdout.close()  # reaching EOF doesn't implicitly close
-            else:
-                xml_out += newData
-        stdout.channel.close()
-        stderr.channel.close()
+
+        # Convenience wrapper for reading data from stdout/stderr
+        # Returns empty binary string if EOF *or* timeout (no data)
+        # Closes file on EOF
+        def read_data(f):
+            if f.closed:
+                return b""
+
+            try:
+                data = f.read()
+                if not data:
+                    f.close()
+                return data
+            except socket.timeout:
+                return b""
+
+        while not all((stdout.closed, stderr.closed)):
+            xml_out += read_data(stdout)
+            err_out += read_data(stderr)
+
+        channel.close()
         return xml_out
 
     def _close(self):

tests/test_unit_transport_ssh.py

@@ -30,7 +30,10 @@ def test_ssh_transport_minimal(mocker):
     assert isinstance(out, (bytes, str))
 
     command = "/QOpenSys/pkgs/bin/xmlservice-cli"
-    ssh_client.exec_command.assert_called_once_with(command)
+    ssh_client.exec_command.assert_called_once()
+
+    args = ssh_client.exec_command.call_args
+    assert args[0] == (command,)
 
 
 def test_ssh_transport_raises_when_closed(mocker):