From 4da8a3d59dbf62205075ccd8e1694aba8744262b Mon Sep 17 00:00:00 2001
From: Josip Sokcevic <sokcevic@google.com>
Date: Mon, 11 Oct 2021 22:37:54 +0000
Subject: [PATCH] Fix bot_update hanging on exception

This change starts observers as soon as possible, and wraps the entire
block that can cause an exception in try..finally. This ensures that we
stop threads are stopped.

If we notice the same behavior, it could be worth to put a cap on number
of executions per observer, and shut the thread down once reached.

R=apolito@google.com

Bug: 1255228
Change-Id: I1f165267460da02b3bbba39022c262e6c29fe86b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3213451
Reviewed-by: Anthony Polito <apolito@google.com>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
---
 .../bot_update/resources/bot_update.py        | 63 +++++++++++--------
 tests/bot_update_coverage_test.py             |  6 +-
 2 files changed, 41 insertions(+), 28 deletions(-)

diff --git a/recipes/recipe_modules/bot_update/resources/bot_update.py b/recipes/recipe_modules/bot_update/resources/bot_update.py
index 08d92ed08..8ccb2112c 100755
--- a/recipes/recipe_modules/bot_update/resources/bot_update.py
+++ b/recipes/recipe_modules/bot_update/resources/bot_update.py
@@ -134,6 +134,7 @@ class RepeatingTimer(threading.Thread):
     self.kwargs = kwargs if kwargs is not None else {}
     self.cond = threading.Condition()
     self.is_shutdown = False
+    self.is_reset = False
 
   def reset(self):
     """Resets timer interval."""
@@ -191,6 +192,8 @@ def call(*args, **kwargs):  # pragma: no cover
   env = os.environ.copy()
   env.update(new_env)
   kwargs['env'] = env
+  stale_process_duration = env.get('STALE_PROCESS_DURATION',
+                                   STALE_PROCESS_DURATION)
 
   if new_env:
     print('===Injecting Environment Variables===')
@@ -200,40 +203,46 @@ def call(*args, **kwargs):  # pragma: no cover
   print('In directory: %s' % cwd)
   start_time = time.time()
   proc = subprocess.Popen(args, **kwargs)
-  if stdin_data:
-    proc.stdin.write(stdin_data)
-    proc.stdin.close()
-  stale_process_duration = env.get('STALE_PROCESS_DURATION',
-                                   STALE_PROCESS_DURATION)
   observers = [
       RepeatingTimer(300, _print_pstree),
       RepeatingTimer(int(stale_process_duration), _terminate_process, [proc])]
+
   for observer in observers:
     observer.start()
-  # This is here because passing 'sys.stdout' into stdout for proc will
-  # produce out of order output.
-  hanging_cr = False
-  while True:
-    for observer in observers:
-      observer.reset()
-    buf = proc.stdout.read(BUF_SIZE)
-    if not buf:
-      break
-    if hanging_cr:
-      buf = '\r' + buf
-    hanging_cr = buf.endswith('\r')
+
+  try:
+    # If there's an exception in this block, we need to stop all observers.
+    # Otherwise, observers will be spinning and main process won't exit while
+    # the main thread will be doing nothing.
+    if stdin_data:
+      proc.stdin.write(stdin_data)
+      proc.stdin.close()
+    # This is here because passing 'sys.stdout' into stdout for proc will
+    # produce out of order output.
+    hanging_cr = False
+    while True:
+      for observer in observers:
+        observer.reset()
+      buf = proc.stdout.read(BUF_SIZE)
+      if not buf:
+        break
+      if hanging_cr:
+        buf = '\r' + buf
+      hanging_cr = buf.endswith(b'\r')
+      if hanging_cr:
+        buf = buf[:-1]
+      buf = buf.replace(b'\r\n', b'\n').replace(b'\r', b'\n')
+      _stdout_write(buf)
+      out.write(buf)
     if hanging_cr:
-      buf = buf[:-1]
-    buf = buf.replace('\r\n', '\n').replace('\r', '\n')
-    _stdout_write(buf)
-    out.write(buf)
-  if hanging_cr:
-    _stdout_write('\n')
-    out.write('\n')
-  for observer in observers:
-    observer.shutdown()
+      _stdout_write(b'\n')
+      out.write(b'\n')
+
+    code = proc.wait()
+  finally:
+    for observer in observers:
+      observer.shutdown()
 
-  code = proc.wait()
   elapsed_time = ((time.time() - start_time) / 60.0)
   outval = out.getvalue().decode('utf-8')
   if code:
diff --git a/tests/bot_update_coverage_test.py b/tests/bot_update_coverage_test.py
index 1c86d492d..cbd9fb94f 100755
--- a/tests/bot_update_coverage_test.py
+++ b/tests/bot_update_coverage_test.py
@@ -251,7 +251,7 @@ class BotUpdateUnittests(unittest.TestCase):
     bot_update.ensure_checkout(**self.params)
     args = self.gclient.records[0]
     patch_refs = set(
-        args[i+1] for i in xrange(len(args))
+        args[i+1] for i in range(len(args))
         if args[i] == '--patch-ref' and i+1 < len(args))
     self.assertIn(self.params['patch_refs'][0], patch_refs)
     self.assertIn(self.params['patch_refs'][1], patch_refs)
@@ -286,6 +286,10 @@ class BotUpdateUnittests(unittest.TestCase):
     actual_results = bot_update.parse_revisions(revisions, 'root')
     self.assertEqual(expected_results, actual_results)
 
+class CallUnitTest(unittest.TestCase):
+  def testCall(self):
+    ret = bot_update.call(sys.executable, '-c', 'print(1)')
+    self.assertEqual(u'1\n', ret)
 
 if __name__ == '__main__':
   unittest.main()