Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3d572940 authored by Tom Cherry's avatar Tom Cherry
Browse files

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
parent b15429c0

base/include/android-base/properties.h

+4 −5
Original line number Original line Diff line number Diff line
@@ -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`.

// Waits for the system property `key` to have the value `expected_value`.

// Times out after `relative_timeout`.

// Times out after `relative_timeout`.

// Returns true on success, false on timeout.

// Returns true on success, false on timeout.

bool WaitForProperty(const std::string& key,

bool WaitForProperty(const std::string& key, const std::string& expected_value,

                     const std::string& expected_value,

                     std::chrono::milliseconds relative_timeout = std::chrono::milliseconds::max());

                     std::chrono::milliseconds relative_timeout);





// Waits for the system property `key` to be created.

// Waits for the system property `key` to be created.

// Times out after `relative_timeout`.

// Times out after `relative_timeout`.

// Returns true on success, false on timeout.

// Returns true on success, false on timeout.

bool WaitForPropertyCreation(const std::string& key,

bool WaitForPropertyCreation(const std::string& key, std::chrono::milliseconds relative_timeout =

                             std::chrono::milliseconds relative_timeout);

                                                         std::chrono::milliseconds::max());





} // namespace base

} // namespace base

} // namespace android

} // namespace android

base/properties.cpp

+15 −18
Original line number Original line Diff line number Diff line
@@ -101,22 +101,24 @@ static void WaitForPropertyCallback(void* data_ptr, const char*, const char* val 
}

}





// TODO: chrono_utils?

// 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<std::chrono::seconds>(d);

  auto s = std::chrono::duration_cast<std::chrono::seconds>(d);

  auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(d - s);

  auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(d - s);

  ts.tv_sec = s.count();

  ts.tv_sec = s.count();

  ts.tv_nsec = ns.count();

  ts.tv_nsec = ns.count();

}

}





// TODO: boot_clock?

using AbsTime = std::chrono::time_point<std::chrono::steady_clock>;

using AbsTime = std::chrono::time_point<std::chrono::steady_clock>;





static void UpdateTimeSpec(timespec& ts,

static void UpdateTimeSpec(timespec& ts, std::chrono::milliseconds relative_timeout,

                           const AbsTime& timeout) {

                           const AbsTime& start_time) {

  auto now = std::chrono::steady_clock::now();

  auto now = std::chrono::steady_clock::now();

  auto remaining_timeout = std::chrono::duration_cast<std::chrono::nanoseconds>(timeout - now);

  auto time_elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(now - start_time);

  if (remaining_timeout < 0ns) {

  if (time_elapsed >= relative_timeout) {

    ts = { 0, 0 };

    ts = { 0, 0 };

  } else {

  } else {

    auto remaining_timeout = relative_timeout - time_elapsed;

    DurationToTimeSpec(ts, remaining_timeout);

    DurationToTimeSpec(ts, remaining_timeout);

  }

  }

}

}
@@ -127,11 +129,7 @@ static void UpdateTimeSpec(timespec& ts, 
// Returns nullptr on timeout.

// Returns nullptr on timeout.

static const prop_info* WaitForPropertyCreation(const std::string& key,

static const prop_info* WaitForPropertyCreation(const std::string& key,

                                                const std::chrono::milliseconds& relative_timeout,

                                                const std::chrono::milliseconds& relative_timeout,

                                                AbsTime& absolute_timeout) {

                                                const AbsTime& start_time) {

  // TODO: boot_clock?

  auto now = std::chrono::steady_clock::now();

  absolute_timeout = now + relative_timeout;



  // Find the property's prop_info*.

  // Find the property's prop_info*.

  const prop_info* pi;

  const prop_info* pi;

  unsigned global_serial = 0;

  unsigned global_serial = 0;
@@ -139,17 +137,16 @@ static const prop_info* WaitForPropertyCreation(const std::string& key, 
    // The property doesn't even exist yet.

    // The property doesn't even exist yet.

    // Wait for a global change and then look again.

    // Wait for a global change and then look again.

    timespec ts;

    timespec ts;

    UpdateTimeSpec(ts, absolute_timeout);

    UpdateTimeSpec(ts, relative_timeout, start_time);

    if (!__system_property_wait(nullptr, global_serial, &global_serial, &ts)) return nullptr;

    if (!__system_property_wait(nullptr, global_serial, &global_serial, &ts)) return nullptr;

  }

  }

  return pi;

  return pi;

}

}





bool WaitForProperty(const std::string& key,

bool WaitForProperty(const std::string& key, const std::string& expected_value,

                     const std::string& expected_value,

                     std::chrono::milliseconds relative_timeout) {

                     std::chrono::milliseconds relative_timeout) {

  AbsTime absolute_timeout;

  auto start_time = std::chrono::steady_clock::now();

  const prop_info* pi = WaitForPropertyCreation(key, relative_timeout, absolute_timeout);

  const prop_info* pi = WaitForPropertyCreation(key, relative_timeout, start_time);

  if (pi == nullptr) return false;

  if (pi == nullptr) return false;





  WaitForPropertyData data;

  WaitForPropertyData data;
@@ -162,7 +159,7 @@ bool WaitForProperty(const std::string& key, 
    if (data.done) return true;

    if (data.done) return true;





    // It didn't, so wait for the property to change before checking again.

    // 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;

    uint32_t unused;

    if (!__system_property_wait(pi, data.last_read_serial, &unused, &ts)) return false;

    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,

bool WaitForPropertyCreation(const std::string& key,

                             std::chrono::milliseconds relative_timeout) {

                             std::chrono::milliseconds relative_timeout) {

  AbsTime absolute_timeout;

  auto start_time = std::chrono::steady_clock::now();

  return (WaitForPropertyCreation(key, relative_timeout, absolute_timeout) != nullptr);

  return (WaitForPropertyCreation(key, relative_timeout, start_time) != nullptr);

}

}





}  // namespace base

}  // namespace base

base/properties_test.cpp

+32 −0
Original line number Original line Diff line number Diff line
@@ -151,6 +151,38 @@ TEST(properties, WaitForProperty_timeout) { 
  ASSERT_LT(std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0), 600ms);

  ASSERT_LT(std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0), 600ms);

}

}





TEST(properties, WaitForProperty_MaxTimeout) {

  std::atomic<bool> 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<bool> 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) {

TEST(properties, WaitForPropertyCreation) {

  std::thread thread([&]() {

  std::thread thread([&]() {

    std::this_thread::sleep_for(100ms);

    std::this_thread::sleep_for(100ms);