Make TimeoutRetryThread's stoppable

build/android/pylib/utils/timeout_retry.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """A utility to run functions with timeouts and retries."""
 # pylint: disable=W0702
 
 import threading
 
 from pylib.utils import reraiser_thread
 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):
   """Runs the passed function in a separate thread with timeouts and retries.
 
   Args:
     func: the function to be wrapped.
     timeout: the timeout in seconds for each try.
     retries: the number of retries.
     args: list of positional args to pass to |func|.
     kwargs: dictionary of keyword args to pass to |func|.
 
   Returns:
     The return value of func(*args, **kwargs).
   """
   if not args:
     args = []
   if not kwargs:
     kwargs = {}
 
   # The return value uses a list because Python variables are references, not
   # values. Closures make a copy of the reference, so updating the closure's
   # reference wouldn't update where the original reference pointed.
   ret = [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