Refactor: Improve subprocess timeout handling

This refactors `run_command` to use `subprocess.Popen.communicate(timeout=...)` for a more robust timeout mechanism.

The old method, relying on a custom `threading.Timer`, could fail to terminate stubborn processes like `fastboot oem dmesg` which ignore `SIGTERM`. The new approach uses `communicate()`, which sends a forceful `SIGKILL` on timeout, guaranteeing the process is terminated.

This change also updates the code to use the modern `text` argument introduced in Python 3.7 to replace `universal_newlines`.

Author: ko1in1u

mobly/utils.py

@@ -474,36 +474,23 @@ def run_command(
       shell=shell,
       cwd=cwd,
       env=env,
-      universal_newlines=universal_newlines,
+      text=universal_newlines,  # "text" is introdcued in Python 3.7.
   )
-  timer = None
-  timer_triggered = threading.Event()
-  if timeout and timeout > 0:
-    # The wait method on process will hang when used with PIPEs with large
-    # outputs, so use a timer thread instead.
-
-    def timeout_expired():
-      timer_triggered.set()
-      process.terminate()
-
-    timer = threading.Timer(timeout, timeout_expired)
-    timer.start()
-  # If the command takes longer than the timeout, then the timer thread
-  # will kill the subprocess, which will make it terminate.
-  out, err = process.communicate()
-  if timer is not None:
-    timer.cancel()
-  if timer_triggered.is_set():
-    raise subprocess.TimeoutExpired(
-        cmd=cmd, timeout=timeout, output=out, stderr=err
+  out, err = None, None
+  try:
+    out, err = process.communicate(timeout=timeout)
+  except subprocess.TimeoutExpired:
+    process.kill()
+    out, err = process.communicate()
+    raise
+  finally:
+    logging.debug(
+        'cmd: %s, stdout: %s, stderr: %s, ret: %s',
+        cli_cmd_to_string(cmd),
+        out,
+        err,
+        process.returncode,
     )
-  logging.debug(
-      'cmd: %s, stdout: %s, stderr: %s, ret: %s',
-      cli_cmd_to_string(cmd),
-      out,
-      err,
-      process.returncode,
-  )
   return process.returncode, out, err
 
 

tests/mobly/utils_test.py

@@ -298,9 +298,8 @@ def test_run_command_with_timeout_expired(self):
     with self.assertRaisesRegex(subprocess.TimeoutExpired, 'sleep'):
       _ = utils.run_command(self.sleep_cmd(4), timeout=0.01)
 
-  @mock.patch('threading.Timer')
   @mock.patch('subprocess.Popen')
-  def test_run_command_with_default_params(self, mock_popen, mock_timer):
+  def test_run_command_with_default_params(self, mock_popen):
     mock_command = mock.MagicMock(spec=dict)
     mock_proc = mock_popen.return_value
     mock_proc.communicate.return_value = ('fake_out', 'fake_err')
@@ -316,13 +315,11 @@ def test_run_command_with_default_params(self, mock_popen, mock_timer):
         shell=False,
         cwd=None,
         env=None,
-        universal_newlines=False,
+        text=False,
     )
-    mock_timer.assert_not_called()
 
-  @mock.patch('threading.Timer')
   @mock.patch('subprocess.Popen')
-  def test_run_command_with_custom_params(self, mock_popen, mock_timer):
+  def test_run_command_with_custom_params(self, mock_popen):
     mock_command = mock.MagicMock(spec=dict)
     mock_stdout = mock.MagicMock(spec=int)
     mock_stderr = mock.MagicMock(spec=int)
@@ -352,9 +349,8 @@ def test_run_command_with_custom_params(self, mock_popen, mock_timer):
         shell=mock_shell,
         cwd=None,
         env=mock_env,
-        universal_newlines=mock_universal_newlines,
+        text=mock_universal_newlines,
     )
-    mock_timer.assert_called_with(1234, mock.ANY)
 
   def test_run_command_with_universal_newlines_false(self):
     _, out, _ = utils.run_command(