use upper limit for output data in Command (#4475)

fixes #4474

Author: vladak

tools/src/main/python/opengrok_tools/utils/command.py

@@ -51,7 +51,8 @@ class Command:
 
     def __init__(self, cmd, args_subst=None, args_append=None, logger=None,
                  excl_subst=False, work_dir=None, env_vars=None, timeout=None,
-                 redirect_stderr=True, resource_limits=None, doprint=False):
+                 redirect_stderr=True, resource_limits=None, doprint=False,
+                 max_line_length=250, max_lines=10000):
 
         if doprint is None:
             doprint = False
@@ -71,6 +72,8 @@ def __init__(self, cmd, args_subst=None, args_append=None, logger=None,
         self.doprint = doprint
         self.err = None
         self.returncode = None
+        self.max_line_length = int(max_line_length)
+        self.max_lines = int(max_lines)
 
         self.logger = logger or logging.getLogger(__name__)
 
@@ -159,14 +162,18 @@ class OutputThread(threading.Thread):
             stdout/stderr buffers fill up.
             """
 
-            def __init__(self, event, logger, doprint=False):
+            def __init__(self, event, logger, doprint=False, max_line_length=250, max_lines=10000):
                 super(OutputThread, self).__init__()
                 self.read_fd, self.write_fd = os.pipe()
                 self.pipe_fobj = os.fdopen(self.read_fd, encoding='utf8')
                 self.out = []
                 self.event = event
                 self.logger = logger
                 self.doprint = doprint
+                # Convert the maximums to integers to avoid exceptions when using them as indexes
+                # in case they are passed as floats.
+                self.max_line_length = int(max_line_length)
+                self.max_lines = int(max_lines)
 
                 # Start the thread now.
                 self.start()
@@ -185,17 +192,31 @@ def run(self):
                         self.event.set()
                         return
 
+                    line = line.rstrip()    # This will remove not only newline but also whitespace.
+
+                    # Assuming that self.max_line_length is bigger than 3.
+                    if len(line) > self.max_line_length:
+                        line = line[:self.max_line_length] + "..."
+
+                    # Shorten the list to be one less than the maximum because a line is going to be added.
+                    if len(self.out) >= self.max_lines:
+                        self.out = self.out[-self.max_lines + 1:]
+                        self.out[0] = "... <truncated>"
+
                     self.out.append(line)
 
                     if self.doprint:
                         # Even if logging below fails, the thread has to keep
                         # running to avoid hangups of the executed command.
                         try:
-                            self.logger.info(line.rstrip())
+                            self.logger.info(line)
                         except Exception as print_exc:
                             self.logger.error(print_exc)
 
             def getoutput(self):
+                """
+                :return: list of lines with trailing whitespace (including newlines) stripped
+                """
                 return self.out
 
             def fileno(self):
@@ -226,18 +247,21 @@ def close(self):
         timeout_thread = None
         output_event = threading.Event()
         output_thread = OutputThread(output_event, self.logger,
-                                     doprint=self.doprint)
+                                     doprint=self.doprint,
+                                     max_lines=self.max_lines,
+                                     max_line_length=self.max_line_length)
 
-        # If stderr redirection is off, setup a thread that will capture
-        # stderr data.
+        # If stderr redirection is off, set up a thread that will capture stderr data.
         stderr_thread = None
         stderr_event = None
         if self.redirect_stderr:
             stderr_dest = subprocess.STDOUT
         else:
             stderr_event = threading.Event()
             stderr_thread = OutputThread(stderr_event, self.logger,
-                                         doprint=self.doprint)
+                                         doprint=self.doprint,
+                                         max_lines=self.max_lines,
+                                         max_line_length=self.max_line_length)
             stderr_dest = stderr_thread
 
         start_time = None
@@ -403,11 +427,18 @@ def getretcode(self):
 
     def getoutputstr(self):
         if self.state == Command.FINISHED:
-            return "".join(self.out).strip()
+            s = os.linesep.join(self.out)
+            if self.out:
+                s += os.linesep
+            return s
         else:
             return None
 
     def getoutput(self):
+        """
+        :return: list of lines (with trailing whitespace and newlines stripped)
+        or None if the command has not finished yet
+        """
         if self.state == Command.FINISHED:
             return self.out
         else:
@@ -418,7 +449,10 @@ def geterroutput(self):
 
     def geterroutputstr(self):
         if self.err:
-            return "".join(self.err).strip()
+            s = os.linesep.join(self.err)
+            if len(self.err) > 0:
+                s += os.linesep
+            return s
         else:
             return ""
 

tools/src/test/python/test_command.py

@@ -96,13 +96,15 @@ def test_execute_nonexistent():
     assert cmd.getstate() == Command.ERRORED
 
 
+# Uses /bin/ls, therefore Unix only.
 @posix_only
 def test_getoutput():
     cmd = Command(['/bin/ls', '/etc/passwd'])
     cmd.execute()
-    assert cmd.getoutput() == ['/etc/passwd\n']
+    assert cmd.getoutput() == ['/etc/passwd']
 
 
+# Uses /bin/ls, therefore Unix only.
 @posix_only
 def test_work_dir():
     os.chdir("/")
@@ -118,7 +120,7 @@ def test_work_dir():
 def test_env(env_binary):
     cmd = Command([env_binary], env_vars={'FOO': 'BAR', 'A': 'B'})
     cmd.execute()
-    assert "FOO=BAR\n" in cmd.getoutput()
+    assert "FOO=BAR" in cmd.getoutput()
 
 
 @system_binary('true')
@@ -181,16 +183,17 @@ def test_stderr():
 
 # This test needs the "/bin/cat" command, therefore it is Unix only.
 @posix_only
-def test_long_output():
+@pytest.mark.parametrize('shorten', [True, False])
+def test_long_output(shorten):
     """
-    Test that output thread in the Command class captures all of the output.
+    Test that output thread in the Command class captures all the output.
     (and also it does not hang the command by filling up the pipe)
 
-    By default stderr is redirected to stdout.
+    By default, stderr is redirected to stdout.
     """
-    # in bytes, should be enough to fill a pipe
     num_lines = 5000
     line_length = 1000
+    # should be enough to fill a pipe
     num_bytes = num_lines * (line_length + 1)
     with tempfile.NamedTemporaryFile() as file:
         for _ in range(num_lines):
@@ -199,13 +202,22 @@ def test_long_output():
         file.flush()
         assert os.path.getsize(file.name) == num_bytes
 
-        cmd = Command(["/bin/cat", file.name])
+        if shorten:
+            num_lines //= 2
+            line_length //= 2
+        cmd = Command(["/bin/cat", file.name],
+                      max_lines=num_lines, max_line_length=line_length)
         cmd.execute()
 
         assert cmd.getstate() == Command.FINISHED
         assert cmd.getretcode() == 0
         assert cmd.geterroutput() is None
-        assert len("".join(cmd.getoutput())) == num_bytes
+        assert len(cmd.getoutput()) == num_lines
+        if shorten:
+            # Add 3 for the '...' suffix.
+            assert all(len(x) <= line_length + 3 for x in cmd.getoutput())
+        else:
+            assert len("\n".join(cmd.getoutput()) + "\n") == num_bytes
 
 
 @posix_only

tools/src/test/python/test_command_sequence.py

@@ -173,7 +173,7 @@ def test_project_subst():
     cmds = CommandSequence(CommandSequenceBase("test-subst", cmd_list))
     cmds.run()
 
-    assert cmds.outputs['/bin/echo test-subst'] == ['test-subst\n']
+    assert cmds.outputs['/bin/echo test-subst'] == ['test-subst']
 
 
 @pytest.mark.skipif(not os.path.exists('/bin/echo'),
@@ -184,7 +184,7 @@ def test_args_subst():
     cmds = CommandSequence(CommandSequenceBase("test-subst", cmd_list))
     cmds.run()
 
-    assert cmds.outputs['/bin/echo foo'] == ['foo\n']
+    assert cmds.outputs['/bin/echo foo'] == ['foo']
 
 
 @pytest.mark.skipif(not os.path.exists('/bin/echo'),
@@ -197,7 +197,7 @@ def test_args_subst_env():
     cmds.run()
     os.environ.pop("FOO")
 
-    assert cmds.outputs['/bin/echo bar'] == ['bar\n']
+    assert cmds.outputs['/bin/echo bar'] == ['bar']
 
 
 def test_cleanup_exception():