Make TimeoutRetryThread's stoppable

build/android/pylib/utils/reraiser_thread.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.
 
 """Thread and ThreadGroup that reraise exceptions on the main thread."""
 # pylint: disable=W0212
 
 import logging
 import sys
 import threading
@@ skipping 40 matching lines @@
     if not args:
       args = []
     if not kwargs:
       kwargs = {}
     self.daemon = True
     self._func = func
     self._args = args
     self._kwargs = kwargs
     self._ret = None
     self._exc_info = None
+    self._stopped = False
 
   def ReraiseIfException(self):
     """Reraise exception if an exception was raised in the thread."""
     if self._exc_info:
       raise self._exc_info[0], self._exc_info[1], self._exc_info[2]
 
   def GetReturnValue(self):
     """Reraise exception if present, otherwise get the return value."""
     self.ReraiseIfException()
     return self._ret
 
+  def StopThread(self):

[jbudorick, 2014/10/24 17:30:51]

I don't think we want to provide this in reraiser_thread unless we can actually
make reraiser_thread stop the thread. Relying on self._func to check Stopped
seems problematic.

I don't have any great suggestions for implementing this otherwise, but I'll
keep thinking about how we could.

[perezju, 2014/10/27 11:07:07]

Looking around, e.g.
http://stackoverflow.com/questions/323972/is-there-any-way-to-kill-a-thread-i...,
asking a thread to stop is perhaps the least problematic approach. (And
definitely better than what we now do, which is... nothing and keep running 2 or
3 threads at the same time during retries).

There seems to be a hack using PyThreadState_SetAsyncExc to make one thread
raise an exception on another, but besides risky also seems quite useless if the
following is true: "If the thread is busy in a system call (time.sleep(),
socket.accept(), ...), the exception is simply ignored.".

[jbudorick, 2014/10/27 19:15:36]

I'm not sure this is better than what we do now. Is there some way that we can
either guarantee that reraiser threads check to see if they should stop or only
make this interface available if the thread supports stopping? If not, this just
seems liable to deceive clients.

We should also rename this to make it clear that this merely asks the thread to
stop.
Yeah, I don't like that here.

[perezju, 2014/10/28 14:31:17]

Just for fun, trying to reboot with a very low timeout.

This is what the TimeoutRetry wrapper currently does:
https://drive.google.com/a/chromium.org/file/d/0B7JfGcdoP-YzTnAzOEJRUXFhNmM/v...

This is what I think it should do:
https://drive.google.com/a/chromium.org/file/d/0B7JfGcdoP-YzQlZIWnEyLTZ1TXM/v...

+    """Request this thread to stop."""
+    self._stopped = True
+
+  def Stopped(self):
+    return self._stopped
+
   #override
   def run(self):
     """Overrides Thread.run() to add support for reraising exceptions."""
     try:
       self._ret = self._func(*self._args, **self._kwargs)
     except: # pylint: disable=W0702
       self._exc_info = sys.exc_info()
 
 
 class ReraiserThreadGroup(object):
@@ skipping 15 matching lines @@
     Args:
       thread: a ReraiserThread object.
     """
     self._threads.append(thread)
 
   def StartAll(self):
     """Start all threads."""
     for thread in self._threads:
       thread.start()
 
-  def _JoinAll(self, watcher=watchdog_timer.WatchdogTimer(None)):
+  def _JoinAll(self, watcher):
     """Join all threads without stack dumps.
 
     Reraises exceptions raised by the child threads and supports breaking
     immediately on exceptions raised on the main thread.
 
     Args:
       watcher: Watchdog object providing timeout, by default waits forever.
     """
     alive_threads = self._threads[:]
     while alive_threads:
       for thread in alive_threads[:]:
         if watcher.IsTimedOut():
           raise TimeoutError('Timed out waiting for %d of %d threads.' %
                              (len(alive_threads), len(self._threads)))
         # Allow the main thread to periodically check for interrupts.
         thread.join(0.1)
         if not thread.isAlive():
           alive_threads.remove(thread)
     # All threads are allowed to complete before reraising exceptions.
     for thread in self._threads:
       thread.ReraiseIfException()
 
-  def JoinAll(self, watcher=watchdog_timer.WatchdogTimer(None)):
+  def JoinAll(self, watcher=None):
     """Join all threads.
 
     Reraises exceptions raised by the child threads and supports breaking
     immediately on exceptions raised on the main thread. Unfinished threads'
     stacks will be logged on watchdog timeout.
 
     Args:
       watcher: Watchdog object providing timeout, by default waits forever.
     """
+    if watcher is None:
+      watcher=watchdog_timer.WatchdogTimer(None)

[jbudorick, 2014/10/24 17:30:52]

nice catch.

nit: spaces around =

     try:
       self._JoinAll(watcher)
     except TimeoutError:
       for thread in (t for t in self._threads if t.isAlive()):
         LogThreadStack(thread)
+        thread.StopThread()
       raise
 
   def GetAllReturnValues(self, watcher=watchdog_timer.WatchdogTimer(None)):
     """Get all return values, joining all threads if necessary.
 
     Args:
       watcher: same as in |JoinAll|. Only used if threads are alive.
     """
     if any([t.isAlive() for t in self._threads]):
       self.JoinAll(watcher)
     return [t.GetReturnValue() for t in self._threads]