From 52687eb3b9aa8982cab5e11fae2ae879c6bc1b69 Mon Sep 17 00:00:00 2001 From: David Reiss Date: Thu, 4 Jun 2009 00:32:57 +0000 Subject: [PATCH] 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 --- lib/cpp/src/concurrency/TimerManager.cpp | 7 ++++++- lib/cpp/src/concurrency/test/TimerManagerTests.h | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/cpp/src/concurrency/TimerManager.cpp b/lib/cpp/src/concurrency/TimerManager.cpp index 003e3155..0540d9c7 100644 --- a/lib/cpp/src/concurrency/TimerManager.cpp +++ b/lib/cpp/src/concurrency/TimerManager.cpp @@ -239,13 +239,18 @@ void TimerManager::add(shared_ptr 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 >(timeout, shared_ptr(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 e6fe6ce7..b89074c0 100644 --- 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 task = shared_ptr(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 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(); -- 2.17.1