From b257cc1248afc21cf8db42b392f344c17ea0eb17 Mon Sep 17 00:00:00 2001
From: Kolin Lu <kolinlu@google.com>
Date: Wed, 20 Aug 2025 14:18:16 -0700
Subject: [PATCH 1/2] 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`.
---
 mobly/utils.py            | 43 ++++++++++++++-------------------------
 tests/mobly/utils_test.py | 12 ++++-------
 2 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/mobly/utils.py b/mobly/utils.py
index f567c325..7593ddad 100644
--- a/mobly/utils.py
+++ b/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
 
 
diff --git a/tests/mobly/utils_test.py b/tests/mobly/utils_test.py
index 2891b924..84f5afd6 100755
--- a/tests/mobly/utils_test.py
+++ b/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(

From ea210872b85648752d0eb1bc51d9a5e5a5cac950 Mon Sep 17 00:00:00 2001
From: Kolin Lu <kolinlu@google.com>
Date: Mon, 15 Dec 2025 22:50:44 -0800
Subject: [PATCH 2/2] Remove unused import

---
 mobly/utils.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mobly/utils.py b/mobly/utils.py
index 7593ddad..96ae94a8 100644
--- a/mobly/utils.py
+++ b/mobly/utils.py
@@ -27,7 +27,6 @@
 import signal
 import string
 import subprocess
-import threading
 import time
 import traceback
 from typing import Literal, Tuple, overload