svn commit: r781630 - in /incubator/thrift/trunk/lib/cpp/src/concurrency: TimerManager.cpp test/TimerManagerTests.h

[dreiss]

Author: dreiss
Date: Thu Jun  4 00:32:57 2009
New Revision: 781630

URL: http://svn.apache.org/viewvc?rev=781630&view=rev
Log:
THRIFT-469. cpp: Fix a bug in TimerManager::add

The old code didn't notify waiters when the inserted task's timeout
was less than the current timeout because it didn't check the task map
to find the lowest timeout until after the new task was inserted.

Modified:
    incubator/thrift/trunk/lib/cpp/src/concurrency/TimerManager.cpp
    incubator/thrift/trunk/lib/cpp/src/concurrency/test/TimerManagerTests.h

Modified: incubator/thrift/trunk/lib/cpp/src/concurrency/TimerManager.cpp
URL: 
http://svn.apache.org/viewvc/incubator/thrift/trunk/lib/cpp/src/concurrency/TimerManager.cpp?rev=781630&r1=781629&r2=781630&view=diff
==============================================================================
--- incubator/thrift/trunk/lib/cpp/src/concurrency/TimerManager.cpp (original)
+++ incubator/thrift/trunk/lib/cpp/src/concurrency/TimerManager.cpp Thu Jun  4 
00:32:57 2009
@@ -239,13 +239,18 @@
       throw IllegalStateException();
     }
 
+    // If the task map is empty, we will kick the dispatcher for sure. 
Otherwise, we kick him
+    // if the expiration time is shorter than the current value. Need to test 
before we insert,
+    // because the new task might insert at the front.
+    bool notifyRequired = (taskCount_ == 0) ? true : timeout < 
taskMap_.begin()->first;
+
     taskCount_++;
     taskMap_.insert(std::pair<int64_t, shared_ptr<Task> >(timeout, 
shared_ptr<Task>(new Task(task))));
 
     // If the task map was empty, or if we have an expiration that is earlier
     // than any previously seen, kick the dispatcher so it can update its
     // timeout
-    if (taskCount_ == 1 || timeout < taskMap_.begin()->first) {
+    if (notifyRequired) {
       monitor_.notify();
     }
   }

Modified: 
incubator/thrift/trunk/lib/cpp/src/concurrency/test/TimerManagerTests.h
URL: 
http://svn.apache.org/viewvc/incubator/thrift/trunk/lib/cpp/src/concurrency/test/TimerManagerTests.h?rev=781630&r1=781629&r2=781630&view=diff
==============================================================================
--- incubator/thrift/trunk/lib/cpp/src/concurrency/test/TimerManagerTests.h 
(original)
+++ incubator/thrift/trunk/lib/cpp/src/concurrency/test/TimerManagerTests.h Thu 
Jun  4 00:32:57 2009
@@ -106,13 +106,26 @@
 
       assert(timerManager.state() == TimerManager::STARTED);
 
-      shared_ptr<TimerManagerTests::Task> task = 
shared_ptr<TimerManagerTests::Task>(new TimerManagerTests::Task(_monitor, 
timeout));
+      // Don't create task yet, because its constructor sets the expected 
completion time, and we
+      // need to delay between inserting the two tasks into the run queue.
+      shared_ptr<TimerManagerTests::Task> task;
 
       {
         Synchronized s(_monitor);
 
         timerManager.add(orphanTask, 10 * timeout);
 
+        try {
+          // Wait for 1 second in order to give timerManager a chance to start 
sleeping in response
+          // to adding orphanTask. We need to do this so we can verify that 
adding the second task
+          // kicks the dispatcher out of the current wait and starts the new 1 
second wait.
+          _monitor.wait (1000);
+          assert (0 == "ERROR: This wait should time out. TimerManager 
dispatcher may have a problem.");
+        } catch (TimedOutException &ex) {
+        }
+
+        task.reset (new TimerManagerTests::Task(_monitor, timeout));
+
         timerManager.add(task, timeout);
 
         _monitor.wait();