From 3d5729402ed14093cfc5385aa4cc67ce90b9b780 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Fri, 24 Mar 2017 16:52:29 -0700 Subject: [PATCH] Fix timeouts for android::base::WaitForProperty* std::chrono doesn't handle integer overflow, so using std::chrono::milliseconds::max() to indicate an infinite timeout is not handled well in the current code. It causes an 'absolute_timeout' earlier in time than 'now' and causes the associated WaitForProperty* functions to return immediately. Also, any duration_cast from relative_timeout to nanoseconds would cause the same issue, as it would overflow in the conversion and result in an invalid results. This change prevents any duration_casts of relative_timeout to nanoseconds and replaces the logic to wait on an absolute timeout with logic that compares the time elapsed to the provided relative timeout. This change also includes a test that std::chrono::milliseconds::max() does not return immediately and that negative values do return immediately. Test: Boot bullhead + libbase_test Change-Id: I335bfa7ba71e86c20119a0ed46014cad44361162 --- base/include/android-base/properties.h | 9 ++++--- base/properties.cpp | 33 ++++++++++++-------------- base/properties_test.cpp | 32 +++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 23 deletions(-) diff --git a/base/include/android-base/properties.h b/base/include/android-base/properties.h index 4de5e57cc..041586c2c 100644 --- a/base/include/android-base/properties.h +++ b/base/include/android-base/properties.h @@ -62,15 +62,14 @@ bool SetProperty(const std::string& key, const std::string& value); // Waits for the system property `key` to have the value `expected_value`. // Times out after `relative_timeout`. // Returns true on success, false on timeout. -bool WaitForProperty(const std::string& key, - const std::string& expected_value, - std::chrono::milliseconds relative_timeout); +bool WaitForProperty(const std::string& key, const std::string& expected_value, + std::chrono::milliseconds relative_timeout = std::chrono::milliseconds::max()); // Waits for the system property `key` to be created. // Times out after `relative_timeout`. // Returns true on success, false on timeout. -bool WaitForPropertyCreation(const std::string& key, - std::chrono::milliseconds relative_timeout); +bool WaitForPropertyCreation(const std::string& key, std::chrono::milliseconds relative_timeout = + std::chrono::milliseconds::max()); } // namespace base } // namespace android diff --git a/base/properties.cpp b/base/properties.cpp index 32c012830..816bca0eb 100644 --- a/base/properties.cpp +++ b/base/properties.cpp @@ -101,22 +101,24 @@ static void WaitForPropertyCallback(void* data_ptr, const char*, const char* val } // TODO: chrono_utils? -static void DurationToTimeSpec(timespec& ts, std::chrono::nanoseconds d) { +static void DurationToTimeSpec(timespec& ts, const std::chrono::milliseconds d) { auto s = std::chrono::duration_cast(d); auto ns = std::chrono::duration_cast(d - s); ts.tv_sec = s.count(); ts.tv_nsec = ns.count(); } +// TODO: boot_clock? using AbsTime = std::chrono::time_point; -static void UpdateTimeSpec(timespec& ts, - const AbsTime& timeout) { +static void UpdateTimeSpec(timespec& ts, std::chrono::milliseconds relative_timeout, + const AbsTime& start_time) { auto now = std::chrono::steady_clock::now(); - auto remaining_timeout = std::chrono::duration_cast(timeout - now); - if (remaining_timeout < 0ns) { + auto time_elapsed = std::chrono::duration_cast(now - start_time); + if (time_elapsed >= relative_timeout) { ts = { 0, 0 }; } else { + auto remaining_timeout = relative_timeout - time_elapsed; DurationToTimeSpec(ts, remaining_timeout); } } @@ -127,11 +129,7 @@ static void UpdateTimeSpec(timespec& ts, // Returns nullptr on timeout. static const prop_info* WaitForPropertyCreation(const std::string& key, const std::chrono::milliseconds& relative_timeout, - AbsTime& absolute_timeout) { - // TODO: boot_clock? - auto now = std::chrono::steady_clock::now(); - absolute_timeout = now + relative_timeout; - + const AbsTime& start_time) { // Find the property's prop_info*. const prop_info* pi; unsigned global_serial = 0; @@ -139,17 +137,16 @@ static const prop_info* WaitForPropertyCreation(const std::string& key, // The property doesn't even exist yet. // Wait for a global change and then look again. timespec ts; - UpdateTimeSpec(ts, absolute_timeout); + UpdateTimeSpec(ts, relative_timeout, start_time); if (!__system_property_wait(nullptr, global_serial, &global_serial, &ts)) return nullptr; } return pi; } -bool WaitForProperty(const std::string& key, - const std::string& expected_value, +bool WaitForProperty(const std::string& key, const std::string& expected_value, std::chrono::milliseconds relative_timeout) { - AbsTime absolute_timeout; - const prop_info* pi = WaitForPropertyCreation(key, relative_timeout, absolute_timeout); + auto start_time = std::chrono::steady_clock::now(); + const prop_info* pi = WaitForPropertyCreation(key, relative_timeout, start_time); if (pi == nullptr) return false; WaitForPropertyData data; @@ -162,7 +159,7 @@ bool WaitForProperty(const std::string& key, if (data.done) return true; // It didn't, so wait for the property to change before checking again. - UpdateTimeSpec(ts, absolute_timeout); + UpdateTimeSpec(ts, relative_timeout, start_time); uint32_t unused; if (!__system_property_wait(pi, data.last_read_serial, &unused, &ts)) return false; } @@ -170,8 +167,8 @@ bool WaitForProperty(const std::string& key, bool WaitForPropertyCreation(const std::string& key, std::chrono::milliseconds relative_timeout) { - AbsTime absolute_timeout; - return (WaitForPropertyCreation(key, relative_timeout, absolute_timeout) != nullptr); + auto start_time = std::chrono::steady_clock::now(); + return (WaitForPropertyCreation(key, relative_timeout, start_time) != nullptr); } } // namespace base diff --git a/base/properties_test.cpp b/base/properties_test.cpp index 1bbe48249..de5f3dcfa 100644 --- a/base/properties_test.cpp +++ b/base/properties_test.cpp @@ -151,6 +151,38 @@ TEST(properties, WaitForProperty_timeout) { ASSERT_LT(std::chrono::duration_cast(t1 - t0), 600ms); } +TEST(properties, WaitForProperty_MaxTimeout) { + std::atomic flag{false}; + std::thread thread([&]() { + android::base::SetProperty("debug.libbase.WaitForProperty_test", "a"); + while (!flag) std::this_thread::yield(); + std::this_thread::sleep_for(500ms); + android::base::SetProperty("debug.libbase.WaitForProperty_test", "b"); + }); + + ASSERT_TRUE(android::base::WaitForProperty("debug.libbase.WaitForProperty_test", "a", 1s)); + flag = true; + // Test that this does not immediately return false due to overflow issues with the timeout. + ASSERT_TRUE(android::base::WaitForProperty("debug.libbase.WaitForProperty_test", "b")); + thread.join(); +} + +TEST(properties, WaitForProperty_NegativeTimeout) { + std::atomic flag{false}; + std::thread thread([&]() { + android::base::SetProperty("debug.libbase.WaitForProperty_test", "a"); + while (!flag) std::this_thread::yield(); + std::this_thread::sleep_for(500ms); + android::base::SetProperty("debug.libbase.WaitForProperty_test", "b"); + }); + + ASSERT_TRUE(android::base::WaitForProperty("debug.libbase.WaitForProperty_test", "a", 1s)); + flag = true; + // Assert that this immediately returns with a negative timeout + ASSERT_FALSE(android::base::WaitForProperty("debug.libbase.WaitForProperty_test", "b", -100ms)); + thread.join(); +} + TEST(properties, WaitForPropertyCreation) { std::thread thread([&]() { std::this_thread::sleep_for(100ms);