Fix for #1012 (process/plugins.py:PerpetualTimer wakes up CPU 20 times per second).

3 files changed, 72 insertions, 11 deletions

diff --git a/cherrypy/process/plugins.py b/cherrypy/process/plugins.py
index 7e5ada21..d8fdc5ea 100644
--- a/cherrypy/process/plugins.py
+++ b/cherrypy/process/plugins.py
@@ -382,7 +382,12 @@ class PIDFile(SimplePlugin):
 
 
 class PerpetualTimer(threading._Timer):
-    """A subclass of threading._Timer whose run() method repeats."""
+    """A responsive subclass of threading._Timer whose run() method repeats.
+    
+    Use this timer only when you really need a very interruptible timer;
+    this checks its 'finished' condition up to 20 times a second, which can
+    results in pretty high CPU usage 
+    """
     
     def run(self):
         while True:
@@ -398,6 +403,45 @@ class PerpetualTimer(threading._Timer):
                 raise
 
 
+class BackgroundTask(threading.Thread):
+    """A subclass of threading.Thread whose run() method repeats.
+    
+    Use this class for most repeating tasks. It uses time.sleep() to wait
+    for each interval, which isn't very responsive; that is, even if you call
+    self.cancel(), you'll have to wait until the sleep() call finishes before
+    the thread stops. To compensate, it defaults to being daemonic, which means
+    it won't delay stopping the whole process.
+    """
+    
+    def __init__(self, interval, function, args=[], kwargs={}):
+        threading.Thread.__init__(self)
+        self.interval = interval
+        self.function = function
+        self.args = args
+        self.kwargs = kwargs
+        self.running = False
+    
+    def cancel(self):
+        self.running = False
+    
+    def run(self):
+        self.running = True
+        while self.running:
+            time.sleep(self.interval)
+            if not self.running:
+                return
+            try:
+                self.function(*self.args, **self.kwargs)
+            except Exception, x:
+                self.bus.log("Error in background task thread function %r." %
+                             self.function, level=40, traceback=True)
+                # Quit on first error to avoid massive logs.
+                raise
+    
+    def _set_daemon(self):
+        return True
+
+
 class Monitor(SimplePlugin):
     """WSPBus listener to periodically run a callback in its own thread.
     
@@ -418,11 +462,11 @@ class Monitor(SimplePlugin):
         self.name = name
     
     def start(self):
-        """Start our callback in its own perpetual timer thread."""
+        """Start our callback in its own background thread."""
         if self.frequency > 0:
             threadname = self.name or self.__class__.__name__
             if self.thread is None:
-                self.thread = PerpetualTimer(self.frequency, self.callback)
+                self.thread = BackgroundTask(self.frequency, self.callback)
                 self.thread.bus = self.bus
                 self.thread.setName(threadname)
                 self.thread.start()
@@ -432,19 +476,26 @@ class Monitor(SimplePlugin):
     start.priority = 70
     
     def stop(self):
-        """Stop our callback's perpetual timer thread."""
+        """Stop our callback's background task thread."""
         if self.thread is None:
             self.bus.log("No thread running for %s." % self.name or self.__class__.__name__)
         else:
             if self.thread is not threading.currentThread():
                 name = self.thread.getName()
                 self.thread.cancel()
-                self.thread.join()
+                if hasattr(threading.Thread, "daemon"):
+                    # Python 2.6+
+                    d = self.thread.daemon
+                else:
+                    d = self.thread.isDaemon()
+                if not d:
+                    self.bus.log("Joining %r" % name)
+                    self.thread.join()
                 self.bus.log("Stopped thread %r." % name)
             self.thread = None
     
     def graceful(self):
-        """Stop the callback's perpetual timer thread and restart it."""
+        """Stop the callback's background task thread and restart it."""
         self.stop()
         self.start()
 
@@ -462,7 +513,7 @@ class Autoreloader(Monitor):
         Monitor.__init__(self, bus, self.run, frequency)
     
     def start(self):
-        """Start our own perpetual timer thread for self.run."""
+        """Start our own background task thread for self.run."""
         if self.thread is None:
             self.mtimes = {}
         Monitor.start(self)
diff --git a/cherrypy/test/test_bus.py b/cherrypy/test/test_bus.py
index 2cc1d444..8a5919d6 100644
--- a/cherrypy/test/test_bus.py
+++ b/cherrypy/test/test_bus.py
@@ -200,14 +200,24 @@ class BusMethodTests(unittest.TestCase):
             time.sleep(0.4)
         threading.Thread(target=f).start()
         threading.Thread(target=g).start()
-        self.assertEqual(len(threading.enumerate()), 3)
+        if hasattr(threading.Thread, "daemon"):
+            # Python 2.6+
+            threads = [t for t in threading.enumerate() if not t.daemon]
+        else:
+            threads = [t for t in threading.enumerate() if not t.isDaemon()]
+        self.assertEqual(len(threads), 3)
 
         b.block()
 
         # The block method MUST wait for the EXITING state.
         self.assertEqual(b.state, b.states.EXITING)
-        # The block method MUST wait for ALL non-main threads to finish.
-        self.assertEqual(len(threading.enumerate()), 1)
+        # The block method MUST wait for ALL non-main, non-daemon threads to finish.
+        if hasattr(threading.Thread, "daemon"):
+            # Python 2.6+
+            threads = [t for t in threading.enumerate() if not t.daemon]
+        else:
+            threads = [t for t in threading.enumerate() if not t.isDaemon()]
+        self.assertEqual(len(threads), 1)
         self.assertLog(['Bus STOPPING', 'Bus STOPPED',
                         'Bus EXITING', 'Bus EXITED',
                         'Waiting for child threads to terminate...'])
diff --git a/cherrypy/test/test_caching.py b/cherrypy/test/test_caching.py
index 0d50fc3e..4739c64c 100644
--- a/cherrypy/test/test_caching.py
+++ b/cherrypy/test/test_caching.py
@@ -303,7 +303,7 @@ class CacheTest(helper.CPWebCase):
             t.join()
         self.assertEqualDates(start, datetime.datetime.now(),
                               # Allow a second for our thread/TCP overhead etc.
-                              seconds=SECONDS + 1)
+                              seconds=SECONDS + 1.1)
     
     def test_cache_control(self):
         self.getPage("/control")