Make TimeoutRetryThread's stoppable

build/android/pylib/utils/timeout_retry.py

Index: build/android/pylib/utils/timeout_retry.py
diff --git a/build/android/pylib/utils/timeout_retry.py b/build/android/pylib/utils/timeout_retry.py
index bae8b16cf7ffd5b16b32e849ffb1af26bf290ead..3a3846d892db401d69aa5351a59333d853171580 100644
--- a/build/android/pylib/utils/timeout_retry.py
+++ b/build/android/pylib/utils/timeout_retry.py
@@ -12,7 +12,55 @@ from pylib.utils import watchdog_timer
 
 
 class TimeoutRetryThread(reraiser_thread.ReraiserThread):

[jbudorick, 2014/10/30 02:27:03]

This interface feels bloated. Why can't we just narrow it down to
CheckTimeout()? Everything else seems ... unnecessary.

[perezju, 2014/10/30 18:44:45]

Yeah ..., have a look at the new one. I hope it's better.

-  pass
+  def __init__(self, func, timeout, name):
+    super(TimeoutRetryThread, self).__init__(func, name=name)
+    self._watcher = watchdog_timer.WatchdogTimer(timeout)
+
+  def GetWatcher(self):
+    """Returns the watchdog timer keeping track of this thread's time."""
+    return self._watcher
+
+  def ElapsedTime(self):
+    """Returns the number of seconds elapsed since the thread started."""
+    return self._watcher.ElapsedTime()
+
+  def RemainingTime(self):
+    """Returns the number of seconds remaining until the thread times out.
+
+    Raises:
+      reraiser_thread.TimeoutError if there is no remaining time.
+    """
+    remaining = self._watcher.RemainingTime()
+    if remaining is not None and remaining <= 0:
+      self.RaiseTimeout()
+    return remaining
+
+  def CheckTimeout(self):
+    """Checks if thread should time out.
+
+    Raises:
+      reraiser_thread.TimeoutError if the thread has timed out.
+    """
+    if self._watcher.IsTimedOut():
+      self.RaiseTimeout()
+
+  def RaiseTimeout(self):
+    """Raises the thread time out error."""
+    reraiser_thread.LogThreadStack(self)

[perezju, 2014/10/29 18:16:50]

Note, we need to log here because the thread is going to kill itself and wont be
"alive" for the reraiser_thread to even notice that a timeout has occurred (as
far as the reraiser_thread is concerned, this thread has probably finished "in
time").

There is small race condition (specially if a thread mixes the new
"TimeoutThread" aware methods with direct low-level IO calls). I'm not entirely
happy about this, but the worst that can happen is that we get the log printed
twice (both from inside and outside the thread).

I'm afraid a real solution would involve some reworking of how reraiser_thread
works, but that's already too much for this CL.

+    raise reraiser_thread.TimeoutError('%s timed out' % self.name)
+
+
+def GetTimeoutThread():

[jbudorick, 2014/10/30 02:27:03]

Nit: rename this, it sounds like it's creating a timeout thread. Maybe
"CurrentTimeoutThread" to mirror current_thread?

(fine with the functionality, though)

[perezju, 2014/10/30 18:44:45]

Done.

+  """Get the current thread if it is a TimeoutRetryThread.
+
+  Returns:
+    The current thread if it is a TimeoutRetryThread, otherwise None.
+  """
+  current_thread = threading.current_thread()
+  if isinstance(current_thread, TimeoutRetryThread):
+    return current_thread
+  else:
+    return None
 
 
 def Run(func, timeout, retries, args=None, kwargs=None):
@@ -40,15 +88,18 @@ def Run(func, timeout, retries, args=None, kwargs=None):
   def RunOnTimeoutThread():
     ret[0] = func(*args, **kwargs)
 
+  num_try = 1

[jbudorick, 2014/10/30 02:27:03]

I'm guessing you're trying to "correct" the retries=3 retrying 3 times for a
total of 4 tries. Leave this the way it is.

Even if we do want to do this, this CL is probably doing too much already. We
can talk about this next week.

[perezju, 2014/10/30 18:44:45]

Mmm, nope, this was also the original behavior. This change is only to count
threads like 1, 2, 3, 4; rather than 3, 2, 1, 0. Because I've also added the
number to the name of the thread, I think this makes more sense when seen in the
logs.

   while True:
+    child_thread = TimeoutRetryThread(
+      RunOnTimeoutThread, timeout,
+      name='TimeoutThread-%d-for-%s' % (num_try,
+                                        threading.current_thread().name))
     try:
-      name = 'TimeoutThread-for-%s' % threading.current_thread().name
-      thread_group = reraiser_thread.ReraiserThreadGroup(
-          [TimeoutRetryThread(RunOnTimeoutThread, name=name)])
+      thread_group = reraiser_thread.ReraiserThreadGroup([child_thread])
       thread_group.StartAll()
-      thread_group.JoinAll(watchdog_timer.WatchdogTimer(timeout))
+      thread_group.JoinAll(child_thread.GetWatcher())

[perezju, 2014/10/29 18:16:50]

This is so that both the thread_group and the timeout_thread use the same
stopwatch. Otherwise we have even more chances of funny race conditions.

       return ret[0]
     except:
-      if retries <= 0:
+      if num_try > retries:
         raise
-      retries -= 1
+      num_try += 1