diff --git a/libc/bionic/posix_timers.cpp b/libc/bionic/posix_timers.cpp index 7ad0ef1fb..3c664d9c8 100644 --- a/libc/bionic/posix_timers.cpp +++ b/libc/bionic/posix_timers.cpp @@ -62,6 +62,7 @@ struct PosixTimer { pthread_t callback_thread; void (*callback)(sigval_t); sigval_t callback_argument; + volatile bool armed; }; static __kernel_timer_t to_kernel_timer_id(timer_t timer) { @@ -83,7 +84,7 @@ static void* __timer_thread_start(void* arg) { continue; } - if (si.si_code == SI_TIMER) { + if (si.si_code == SI_TIMER && timer->armed) { // This signal was sent because a timer fired, so call the callback. timer->callback(timer->callback_argument); } else if (si.si_code == SI_TKILL) { @@ -95,6 +96,9 @@ static void* __timer_thread_start(void* arg) { } static void __timer_thread_stop(PosixTimer* timer) { + // Immediately mark the timer as disarmed so even if some events + // continue to happen, the callback won't be called. + timer->armed = false; pthread_kill(timer->callback_thread, TIMER_SIGNAL); } @@ -121,6 +125,7 @@ int timer_create(clockid_t clock_id, sigevent* evp, timer_t* timer_id) { // Otherwise, this must be SIGEV_THREAD timer... timer->callback = evp->sigev_notify_function; timer->callback_argument = evp->sigev_value; + timer->armed = false; // Check arguments that the kernel doesn't care about but we do. if (timer->callback == NULL) { @@ -200,7 +205,18 @@ int timer_gettime(timer_t id, itimerspec* ts) { // http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_getoverrun.html int timer_settime(timer_t id, int flags, const itimerspec* ts, itimerspec* ots) { - return __timer_settime(to_kernel_timer_id(id), flags, ts, ots); + PosixTimer* timer= reinterpret_cast(id); + int rc = __timer_settime(timer->kernel_timer_id, flags, ts, ots); + if (rc == 0) { + // Mark the timer as either being armed or disarmed. This avoids the + // callback being called after the disarm for SIGEV_THREAD timers only. + if (ts->it_value.tv_sec != 0 || ts->it_value.tv_nsec != 0) { + timer->armed = true; + } else { + timer->armed = false; + } + } + return rc; } // http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_getoverrun.html diff --git a/tests/time_test.cpp b/tests/time_test.cpp index 26c699339..7a551b42a 100644 --- a/tests/time_test.cpp +++ b/tests/time_test.cpp @@ -206,24 +206,46 @@ struct Counter { volatile int value; timer_t timer_id; sigevent_t se; + bool timer_valid; - Counter(void (*fn)(sigval_t)) : value(0) { + Counter(void (*fn)(sigval_t)) : value(0), timer_valid(false) { memset(&se, 0, sizeof(se)); se.sigev_notify = SIGEV_THREAD; se.sigev_notify_function = fn; se.sigev_value.sival_ptr = this; + Create(); } void Create() { + ASSERT_FALSE(timer_valid); ASSERT_EQ(0, timer_create(CLOCK_REALTIME, &se, &timer_id)); + timer_valid = true; + } + + void DeleteTimer() { + ASSERT_TRUE(timer_valid); + ASSERT_EQ(0, timer_delete(timer_id)); + timer_valid = false; } ~Counter() { - if (timer_delete(timer_id) != 0) { - abort(); + if (timer_valid) { + DeleteTimer(); } } + void SetTime(time_t value_s, time_t value_ns, time_t interval_s, time_t interval_ns) { + ::SetTime(timer_id, value_s, value_ns, interval_s, interval_ns); + } + + bool ValueUpdated() { + volatile int current_value = value; + time_t start = time(NULL); + while (current_value == value && (time(NULL) - start) < 5) { + } + return current_value != value; + } + static void CountNotifyFunction(sigval_t value) { Counter* cd = reinterpret_cast(value.sival_ptr); ++cd->value; @@ -234,17 +256,17 @@ struct Counter { ++cd->value; // Setting the initial expiration time to 0 disarms the timer. - SetTime(cd->timer_id, 0, 0, 1, 0); + cd->SetTime(0, 0, 1, 0); } }; TEST(time, timer_settime_0) { Counter counter(Counter::CountAndDisarmNotifyFunction); - counter.Create(); + ASSERT_TRUE(counter.timer_valid); ASSERT_EQ(0, counter.value); - SetTime(counter.timer_id, 0, 1, 1, 0); + counter.SetTime(0, 1, 1, 0); usleep(500000); // The count should just be 1 because we disarmed the timer the first time it fired. @@ -253,15 +275,14 @@ TEST(time, timer_settime_0) { TEST(time, timer_settime_repeats) { Counter counter(Counter::CountNotifyFunction); - counter.Create(); + ASSERT_TRUE(counter.timer_valid); ASSERT_EQ(0, counter.value); - SetTime(counter.timer_id, 0, 1, 0, 10); - usleep(500000); - - // The count should just be > 1 because we let the timer repeat. - ASSERT_GT(counter.value, 1); + counter.SetTime(0, 1, 0, 10); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); } static int timer_create_NULL_signal_handler_invocation_count = 0; @@ -321,17 +342,17 @@ TEST(time, timer_delete_multiple) { TEST(time, timer_create_multiple) { Counter counter1(Counter::CountNotifyFunction); - counter1.Create(); + ASSERT_TRUE(counter1.timer_valid); Counter counter2(Counter::CountNotifyFunction); - counter2.Create(); + ASSERT_TRUE(counter2.timer_valid); Counter counter3(Counter::CountNotifyFunction); - counter3.Create(); + ASSERT_TRUE(counter3.timer_valid); ASSERT_EQ(0, counter1.value); ASSERT_EQ(0, counter2.value); ASSERT_EQ(0, counter3.value); - SetTime(counter2.timer_id, 0, 1, 0, 0); + counter2.SetTime(0, 1, 0, 0); usleep(500000); EXPECT_EQ(0, counter1.value); @@ -425,3 +446,45 @@ TEST(time, clock_nanosleep) { timespec out; ASSERT_EQ(EINVAL, clock_nanosleep(-1, 0, &in, &out)); } + +// Test to verify that disarming a repeatable timer disables the +// callbacks. +TEST(time, timer_disarm_terminates) { + Counter counter(Counter::CountNotifyFunction); + ASSERT_TRUE(counter.timer_valid); + + ASSERT_EQ(0, counter.value); + + counter.SetTime(0, 1, 0, 1); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + + counter.SetTime(0, 0, 1, 0); + volatile int value = counter.value; + usleep(500000); + + // Verify the counter has not been incremented. + ASSERT_EQ(value, counter.value); +} + +// Test to verify that deleting a repeatable timer disables the +// callbacks. +TEST(time, timer_delete_terminates) { + Counter counter(Counter::CountNotifyFunction); + ASSERT_TRUE(counter.timer_valid); + + ASSERT_EQ(0, counter.value); + + counter.SetTime(0, 1, 0, 1); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + + counter.DeleteTimer(); + volatile int value = counter.value; + usleep(500000); + + // Verify the counter has not been incremented. + ASSERT_EQ(value, counter.value); +}