summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPavlin Radoslavov <pavlin@google.com>2016-05-02 10:52:07 -0700
committerPavlin Radoslavov <pavlin@google.com>2016-05-03 17:12:03 -0700
commit1db7f1b0a7d235977ac81623ab06c51df8b493de (patch)
tree1673fb0e7b51ebae23c6ddbe97ec98b3557ed8e7
parent8456e865765b86878f966ceee4a69038c465009e (diff)
downloadandroid-system-bt-1db7f1b0a7d235977ac81623ab06c51df8b493de.tar.gz
android-system-bt-1db7f1b0a7d235977ac81623ab06c51df8b493de.tar.xz
Update alarm_unregister_processing_queue() to cancel scheduled alarms
Update the alarm_unregister_processing_queue() implementation so it cancels all alarms that are scheduled on the corresponding queue. This fixes a race condition during Bluetooth shutdown: if an alarm expires right after an alarm processing queue is invalidated, the alarm processing would try to use the invalidated queue. Added the corresponding unit tests. Also, added a missing call to alarm_unregister_processing_queue(). Bug: 26982349 Change-Id: I09a111e8080b6dbc354dffa03a487f7a8c578ce6
-rw-r--r--bta/sys/bta_sys_main.c1
-rw-r--r--osi/include/alarm.h4
-rw-r--r--osi/src/alarm.c34
-rw-r--r--osi/test/alarm_test.cpp117
4 files changed, 148 insertions, 8 deletions
diff --git a/bta/sys/bta_sys_main.c b/bta/sys/bta_sys_main.c
index f0fbe52..af32cc9 100644
--- a/bta/sys/bta_sys_main.c
+++ b/bta/sys/bta_sys_main.c
@@ -193,6 +193,7 @@ void bta_sys_init(void)
}
void bta_sys_free(void) {
+ alarm_unregister_processing_queue(btu_bta_alarm_queue);
fixed_queue_free(btu_bta_alarm_queue, NULL);
btu_bta_alarm_queue = NULL;
}
diff --git a/osi/include/alarm.h b/osi/include/alarm.h
index 9a4210c..a9934fd 100644
--- a/osi/include/alarm.h
+++ b/osi/include/alarm.h
@@ -101,8 +101,8 @@ bool alarm_is_scheduled(const alarm_t *alarm);
void alarm_register_processing_queue(fixed_queue_t *queue, thread_t *thread);
// Unregisters |queue| for processing alarm callbacks on whichever thread
-// it is registered with. |queue| may not be NULL.
-// This function is idempotent.
+// it is registered with. All alarms currently set for execution on |queue|
+// will be canceled. |queue| may not be NULL. This function is idempotent.
void alarm_unregister_processing_queue(fixed_queue_t *queue);
// Figure out how much time until next expiration.
diff --git a/osi/src/alarm.c b/osi/src/alarm.c
index 669366d..69ded69 100644
--- a/osi/src/alarm.c
+++ b/osi/src/alarm.c
@@ -126,6 +126,7 @@ static period_ms_t now(void);
static void alarm_set_internal(alarm_t *alarm, period_ms_t period,
alarm_callback_t cb, void *data,
fixed_queue_t *queue);
+static void alarm_cancel_internal(alarm_t *alarm);
static void remove_pending_alarm(alarm_t *alarm);
static void schedule_next_instance(alarm_t *alarm);
static void reschedule_root_alarm(void);
@@ -260,7 +261,17 @@ void alarm_cancel(alarm_t *alarm) {
return;
pthread_mutex_lock(&monitor);
+ alarm_cancel_internal(alarm);
+ pthread_mutex_unlock(&monitor);
+
+ // If the callback for |alarm| is in progress, wait here until it completes.
+ pthread_mutex_lock(&alarm->callback_lock);
+ pthread_mutex_unlock(&alarm->callback_lock);
+}
+// Internal implementation of canceling an alarm.
+// The caller must hold the |monitor| lock.
+static void alarm_cancel_internal(alarm_t *alarm) {
bool needs_reschedule = (!list_is_empty(alarms) && list_front(alarms) == alarm);
remove_pending_alarm(alarm);
@@ -270,15 +281,10 @@ void alarm_cancel(alarm_t *alarm) {
alarm->callback = NULL;
alarm->data = NULL;
alarm->stats.canceled_count++;
+ alarm->queue = NULL;
if (needs_reschedule)
reschedule_root_alarm();
-
- pthread_mutex_unlock(&monitor);
-
- // If the callback for |alarm| is in progress, wait here until it completes.
- pthread_mutex_lock(&alarm->callback_lock);
- pthread_mutex_unlock(&alarm->callback_lock);
}
bool alarm_is_scheduled(const alarm_t *alarm) {
@@ -557,7 +563,23 @@ void alarm_register_processing_queue(fixed_queue_t *queue, thread_t *thread) {
}
void alarm_unregister_processing_queue(fixed_queue_t *queue) {
+ assert(alarms != NULL);
+ assert(queue != NULL);
+
fixed_queue_unregister_dequeue(queue);
+
+ // Cancel all alarms that are using this queue
+ pthread_mutex_lock(&monitor);
+ for (list_node_t *node = list_begin(alarms); node != list_end(alarms); ) {
+ alarm_t *alarm = (alarm_t *)list_node(node);
+ node = list_next(node);
+ // TODO: Each module is responsible for tearing down its alarms; currently,
+ // this is not the case. In the future, this check should be replaced by
+ // an assert.
+ if (alarm->queue == queue)
+ alarm_cancel_internal(alarm);
+ }
+ pthread_mutex_unlock(&monitor);
}
static void alarm_queue_ready(fixed_queue_t *queue,
diff --git a/osi/test/alarm_test.cpp b/osi/test/alarm_test.cpp
index 954468c..e4436aa 100644
--- a/osi/test/alarm_test.cpp
+++ b/osi/test/alarm_test.cpp
@@ -336,6 +336,123 @@ TEST_F(AlarmTest, test_callback_ordering_on_queue) {
thread_free(thread);
}
+// Test whether unregistering a processing queue cancels all timers using
+// that queue.
+TEST_F(AlarmTest, test_unregister_processing_queue) {
+ alarm_t *alarms[100];
+ fixed_queue_t *queue = fixed_queue_new(SIZE_MAX);
+ thread_t *thread =
+ thread_new("timers.test_unregister_processing_queue.thread");
+
+ alarm_register_processing_queue(queue, thread);
+
+ for (int i = 0; i < 100; i++) {
+ const std::string alarm_name =
+ "alarm_test.test_unregister_processing_queue[" +
+ std::to_string(i) + "]";
+ alarms[i] = alarm_new(alarm_name.c_str());
+ }
+
+ // Schedule half of the timers to expire soon, and the rest far in the future
+ for (int i = 0; i < 50; i++) {
+ alarm_set_on_queue(alarms[i], 100, ordered_cb, INT_TO_PTR(i), queue);
+ }
+ for (int i = 50; i < 100; i++) {
+ alarm_set_on_queue(alarms[i], 1000 * 1000, ordered_cb, INT_TO_PTR(i), queue);
+ }
+
+ // Wait until half of the timers have expired
+ for (int i = 1; i <= 50; i++) {
+ semaphore_wait(semaphore);
+ EXPECT_GE(cb_counter, i);
+ }
+ EXPECT_EQ(cb_counter, 50);
+ EXPECT_EQ(cb_misordered_counter, 0);
+
+ // Test that only the expired timers are not scheduled
+ for (int i = 0; i < 50; i++) {
+ EXPECT_FALSE(alarm_is_scheduled(alarms[i]));
+ }
+ for (int i = 50; i < 100; i++) {
+ EXPECT_TRUE(alarm_is_scheduled(alarms[i]));
+ }
+
+ alarm_unregister_processing_queue(queue);
+
+ // Test that none of the timers are scheduled
+ for (int i = 0; i < 100; i++) {
+ EXPECT_FALSE(alarm_is_scheduled(alarms[i]));
+ }
+
+ for (int i = 0; i < 100; i++) {
+ alarm_free(alarms[i]);
+ }
+
+ EXPECT_FALSE(WakeLockHeld());
+
+ fixed_queue_free(queue, NULL);
+ thread_free(thread);
+}
+
+// Test whether unregistering a processing queue cancels all periodic timers
+// using that queue.
+TEST_F(AlarmTest, test_periodic_unregister_processing_queue) {
+ alarm_t *alarms[5];
+ fixed_queue_t *queue = fixed_queue_new(SIZE_MAX);
+ thread_t *thread =
+ thread_new("timers.test_periodic_unregister_processing_queue.thread");
+
+ alarm_register_processing_queue(queue, thread);
+
+ for (int i = 0; i < 5; i++) {
+ const std::string alarm_name =
+ "alarm_test.test_periodic_unregister_processing_queue[" +
+ std::to_string(i) + "]";
+ alarms[i] = alarm_new_periodic(alarm_name.c_str());
+ }
+
+ // Schedule each of the timers with different period
+ for (int i = 0; i < 5; i++) {
+ alarm_set_on_queue(alarms[i], 20 + i, cb, INT_TO_PTR(i), queue);
+ }
+ EXPECT_EQ(cb_counter, 0);
+ EXPECT_TRUE(WakeLockHeld());
+
+ for (int i = 1; i <= 20; i++) {
+ semaphore_wait(semaphore);
+
+ EXPECT_GE(cb_counter, i);
+ EXPECT_TRUE(WakeLockHeld());
+ }
+
+ // Test that all timers are still scheduled
+ for (int i = 0; i < 5; i++) {
+ EXPECT_TRUE(alarm_is_scheduled(alarms[i]));
+ }
+
+ alarm_unregister_processing_queue(queue);
+
+ int saved_cb_counter = cb_counter;
+
+ // Test that none of the timers are scheduled
+ for (int i = 0; i < 5; i++) {
+ EXPECT_FALSE(alarm_is_scheduled(alarms[i]));
+ }
+
+ // Sleep for 500ms and test again that the cb_counter hasn't been modified
+ usleep(500 * 1000);
+ EXPECT_TRUE(cb_counter == saved_cb_counter);
+
+ for (int i = 0; i < 5; i++) {
+ alarm_free(alarms[i]);
+ }
+
+ EXPECT_FALSE(WakeLockHeld());
+
+ fixed_queue_free(queue, NULL);
+ thread_free(thread);
+}
+
// Try to catch any race conditions between the timer callback and |alarm_free|.
TEST_F(AlarmTest, test_callback_free_race) {
for (int i = 0; i < 1000; ++i) {