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.

git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@781630 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/lib/cpp/src/concurrency/TimerManager.cpp b/lib/cpp/src/concurrency/TimerManager.cpp
index 003e315..0540d9c 100644 (file)
--- a/lib/cpp/src/concurrency/TimerManager.cpp
+++ b/lib/cpp/src/concurrency/TimerManager.cpp
@@ -239,13 +239,18 @@ void TimerManager::add(shared_ptr<Runnable> task, int64_t timeout) {
       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();
     }
   }

diff --git a/lib/cpp/src/concurrency/test/TimerManagerTests.h b/lib/cpp/src/concurrency/test/TimerManagerTests.h
index e6fe6ce..b89074c 100644 (file)
--- a/lib/cpp/src/concurrency/test/TimerManagerTests.h
+++ b/lib/cpp/src/concurrency/test/TimerManagerTests.h
@@ -106,13 +106,26 @@ class TimerManagerTests {
 
       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();