From bbfc4705d052e1003bd47767085970ceade103c8 Mon Sep 17 00:00:00 2001 From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com> Date: Tue, 17 Aug 2021 10:25:26 -0400 Subject: [PATCH] TODOs from PR #6561 (#8900) * TODOs from PR #6561 #### Problem Some TODOs left over from PR #6561. #### Change overview - Remove the temporary `PrepareEventsWithTimeout()`, and have the tests that used it use a timer instead. - Remove obsolete TODOs. Fixes #7760 _Some unit tests supply a timeout at low level_ Fixes #7756 _add a timer for sleepTime.tv_sec = 10; sleepTime.tv_usec = 0;_ #### Testing Ran the affected tests. * fix kSleepTimeMilliseconds * review --- .../java/CHIPDeviceController-JNI.cpp | 1 - src/inet/tests/TestInetCommon.h | 6 +++--- src/inet/tests/TestInetCommonPosix.cpp | 19 +++++++++---------- src/inet/tests/TestInetEndPoint.cpp | 11 ++++------- src/inet/tests/TestInetLayer.cpp | 7 ++----- src/inet/tests/TestInetLayerDNS.cpp | 12 +++++------- src/inet/tests/TestInetLayerMulticast.cpp | 7 ++----- src/inet/tests/TestLwIPDNS.cpp | 7 ++----- src/system/WatchableEventManagerLibevent.cpp | 9 ++------- src/system/WatchableEventManagerLibevent.h | 3 --- src/system/WatchableEventManagerSelect.cpp | 17 ++++++----------- src/system/WatchableEventManagerSelect.h | 3 --- src/system/tests/TestSystemTimer.cpp | 16 ++++------------ .../raw/tests/NetworkTestHelpers.cpp | 7 ++----- 14 files changed, 41 insertions(+), 84 deletions(-) diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index 60563fd73179c2..cccf036dfb1095 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -1057,7 +1057,6 @@ void * IOThreadMain(void * arg) // Loop until we are told to exit. while (!quit.load(std::memory_order_relaxed)) { - // TODO(#5556): add a timer for `sleepTime.tv_sec = 10; sleepTime.tv_usec = 0;` watchState.PrepareEvents(); // Unlock the stack so that Java threads can make API calls. diff --git a/src/inet/tests/TestInetCommon.h b/src/inet/tests/TestInetCommon.h index 958a4516cced77..8bf55c5dd7f55b 100644 --- a/src/inet/tests/TestInetCommon.h +++ b/src/inet/tests/TestInetCommon.h @@ -63,12 +63,12 @@ void ShutdownSystemLayer(); void InetFailError(CHIP_ERROR err, const char * msg); void InitNetwork(); -void ServiceEvents(struct ::timeval & aSleepTime); +void ServiceEvents(uint32_t aSleepTimeMilliseconds); void ShutdownNetwork(); void DumpMemory(const uint8_t * mem, uint32_t len, const char * prefix, uint32_t rowWidth); void DumpMemory(const uint8_t * mem, uint32_t len, const char * prefix); -inline static void ServiceNetwork(struct ::timeval & aSleepTime) +inline static void ServiceNetwork(uint32_t aSleepTimeMilliseconds) { - ServiceEvents(aSleepTime); + ServiceEvents(aSleepTimeMilliseconds); } diff --git a/src/inet/tests/TestInetCommonPosix.cpp b/src/inet/tests/TestInetCommonPosix.cpp index 33b1b58c0c0010..71cc2dde2611a9 100644 --- a/src/inet/tests/TestInetCommonPosix.cpp +++ b/src/inet/tests/TestInetCommonPosix.cpp @@ -50,7 +50,7 @@ #include #include #include -#include +#include #if CHIP_SYSTEM_CONFIG_USE_LWIP #include @@ -398,10 +398,8 @@ void InitNetwork() while (!NetworkIsReady()) { - struct timeval lSleepTime; - lSleepTime.tv_sec = 0; - lSleepTime.tv_usec = 100000; - ServiceEvents(lSleepTime); + constexpr uint32_t kSleepTimeMilliseconds = 100; + ServiceEvents(kSleepTimeMilliseconds); } // FIXME: this is kinda nasty :( @@ -446,7 +444,7 @@ void InitNetwork() gInet.Init(gSystemLayer, lContext); } -void ServiceEvents(struct ::timeval & aSleepTime) +void ServiceEvents(uint32_t aSleepTimeMilliseconds) { static bool printed = false; @@ -462,8 +460,12 @@ void ServiceEvents(struct ::timeval & aSleepTime) } } + // Start a timer (with a no-op callback) to ensure that WaitForEvents() does not block longer than aSleepTimeMilliseconds. + gSystemLayer.StartTimer( + aSleepTimeMilliseconds, [](System::Layer *, void *) -> void {}, nullptr); + #if CHIP_SYSTEM_CONFIG_USE_SOCKETS - gSystemLayer.WatchableEventsManager().PrepareEventsWithTimeout(aSleepTime); + gSystemLayer.WatchableEventsManager().PrepareEvents(); gSystemLayer.WatchableEventsManager().WaitForEvents(); gSystemLayer.WatchableEventsManager().HandleEvents(); #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS @@ -483,9 +485,6 @@ void ServiceEvents(struct ::timeval & aSleepTime) else sRemainingSystemLayerEventDelay--; - // TODO: Currently timers are delayed by aSleepTime above. A improved solution would have a mechanism to reduce - // aSleepTime according to the next timer. - gSystemLayer.WatchableEventsManager().HandlePlatformTimer(); } } diff --git a/src/inet/tests/TestInetEndPoint.cpp b/src/inet/tests/TestInetEndPoint.cpp index a298cf41fc50d1..90b0cbe7e1cdf0 100644 --- a/src/inet/tests/TestInetEndPoint.cpp +++ b/src/inet/tests/TestInetEndPoint.cpp @@ -132,12 +132,9 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext) char testHostName2[20] = "127.0.0.1"; char testHostName3[20] = ""; char testHostName4[260]; - struct timeval sleepTime; IPAddress testDestAddr[1] = { IPAddress::Any }; CHIP_ERROR err; - - sleepTime.tv_sec = 0; - sleepTime.tv_usec = 10000; + constexpr uint32_t kSleepTimeMilliseconds = 10; memset(testHostName4, 'w', sizeof(testHostName4)); testHostName4[259] = '\0'; @@ -150,7 +147,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext) { while (!callbackHandlerCalled) { - ServiceNetwork(sleepTime); + ServiceNetwork(kSleepTimeMilliseconds); } } @@ -162,7 +159,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext) { while (!callbackHandlerCalled) { - ServiceNetwork(sleepTime); + ServiceNetwork(kSleepTimeMilliseconds); } } @@ -174,7 +171,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext) { while (!callbackHandlerCalled) { - ServiceNetwork(sleepTime); + ServiceNetwork(kSleepTimeMilliseconds); } } diff --git a/src/inet/tests/TestInetLayer.cpp b/src/inet/tests/TestInetLayer.cpp index a7a3bcf4f4cfc9..ddbe1917fc2622 100644 --- a/src/inet/tests/TestInetLayer.cpp +++ b/src/inet/tests/TestInetLayer.cpp @@ -281,14 +281,11 @@ int main(int argc, char * argv[]) while (Common::IsTesting(sTestState.mStatus)) { - struct timeval sleepTime; bool lSucceeded = true; bool lFailed = false; - sleepTime.tv_sec = 0; - sleepTime.tv_usec = 10000; - - ServiceNetwork(sleepTime); + constexpr uint32_t kSleepTimeMilliseconds = 10; + ServiceNetwork(kSleepTimeMilliseconds); CheckSucceededOrFailed(sTestState, lSucceeded, lFailed); diff --git a/src/inet/tests/TestInetLayerDNS.cpp b/src/inet/tests/TestInetLayerDNS.cpp index 227acc78762e7c..8ea2a8437ff40e 100644 --- a/src/inet/tests/TestInetLayerDNS.cpp +++ b/src/inet/tests/TestInetLayerDNS.cpp @@ -652,18 +652,16 @@ static void HandleResolutionComplete(void * appState, CHIP_ERROR err, uint8_t ad } } -static void ServiceNetworkUntilDone(uint32_t timeoutMS) +static void ServiceNetworkUntilDone(uint32_t timeoutMilliseconds) { - uint64_t timeoutTimeMS = System::Clock::GetMonotonicMilliseconds() + timeoutMS; - struct timeval sleepTime; - sleepTime.tv_sec = 0; - sleepTime.tv_usec = 10000; + uint64_t timeoutTimeMilliseconds = System::Clock::GetMonotonicMilliseconds() + timeoutMilliseconds; + constexpr uint32_t kSleepTimeMilliseconds = 10; while (!gDone) { - ServiceNetwork(sleepTime); + ServiceNetwork(kSleepTimeMilliseconds); - if (System::Clock::GetMonotonicMilliseconds() >= timeoutTimeMS) + if (System::Clock::GetMonotonicMilliseconds() >= timeoutTimeMilliseconds) { break; } diff --git a/src/inet/tests/TestInetLayerMulticast.cpp b/src/inet/tests/TestInetLayerMulticast.cpp index ef458af4b42879..6a03656721a6e5 100644 --- a/src/inet/tests/TestInetLayerMulticast.cpp +++ b/src/inet/tests/TestInetLayerMulticast.cpp @@ -318,14 +318,11 @@ int main(int argc, char * argv[]) while (Common::IsTesting(sTestState.mStatus)) { - struct timeval sleepTime; bool lSucceeded = true; bool lFailed = false; - sleepTime.tv_sec = 0; - sleepTime.tv_usec = 10000; - - ServiceNetwork(sleepTime); + constexpr uint32_t kSleepTimeMilliseconds = 10; + ServiceNetwork(kSleepTimeMilliseconds); CheckSucceededOrFailed(sTestState, lSucceeded, lFailed); diff --git a/src/inet/tests/TestLwIPDNS.cpp b/src/inet/tests/TestLwIPDNS.cpp index a8e8580d89b33a..e53824d95b8592 100644 --- a/src/inet/tests/TestLwIPDNS.cpp +++ b/src/inet/tests/TestLwIPDNS.cpp @@ -126,11 +126,8 @@ static void TestLwIPDNS(void) while (!Done) { - struct timeval sleepTime; - sleepTime.tv_sec = 0; - sleepTime.tv_usec = 10000; - - ServiceNetwork(sleepTime); + constexpr uint32_t kSleepTimeMilliseconds = 10; + ServiceNetwork(kSleepTimeMilliseconds); } // Expected cached response diff --git a/src/system/WatchableEventManagerLibevent.cpp b/src/system/WatchableEventManagerLibevent.cpp index 9f0379f50d66ef..bf79e75ad08c19 100644 --- a/src/system/WatchableEventManagerLibevent.cpp +++ b/src/system/WatchableEventManagerLibevent.cpp @@ -89,15 +89,9 @@ CHIP_ERROR WatchableEventManager::Init(System::Layer & systemLayer) } void WatchableEventManager::PrepareEvents() -{ - timeval nextTimeout = { 0, 0 }; - PrepareEventsWithTimeout(nextTimeout); -} - -void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeout) { const Clock::MonotonicMilliseconds currentTime = Clock::GetMonotonicMilliseconds(); - Clock::MonotonicMilliseconds awakenTime = currentTime + TimevalToMilliseconds(nextTimeout); + Clock::MonotonicMilliseconds awakenTime = currentTime; Timer * timer = mTimerList.Earliest(); if (timer && Clock::IsEarlier(timer->mAwakenTime, awakenTime)) @@ -106,6 +100,7 @@ void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeou } const Clock::MonotonicMilliseconds sleepTime = (awakenTime > currentTime) ? (awakenTime - currentTime) : 0; + timeval nextTimeout; MillisecondsToTimeval(sleepTime, nextTimeout); #if CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__ && !__MBED__ diff --git a/src/system/WatchableEventManagerLibevent.h b/src/system/WatchableEventManagerLibevent.h index 346cee2b96d0cc..634162282559c2 100644 --- a/src/system/WatchableEventManagerLibevent.h +++ b/src/system/WatchableEventManagerLibevent.h @@ -61,9 +61,6 @@ class WatchableEventManager void HandleEvents(); void EventLoopEnds() {} - // TODO(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer. - void PrepareEventsWithTimeout(timeval & nextTimeout); - private: /* * In this implementation, libevent invokes LibeventCallbackHandler from beneath WaitForEvents(), diff --git a/src/system/WatchableEventManagerSelect.cpp b/src/system/WatchableEventManagerSelect.cpp index c5dd371488188e..7bda3dc2631332 100644 --- a/src/system/WatchableEventManagerSelect.cpp +++ b/src/system/WatchableEventManagerSelect.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -294,16 +295,10 @@ void WatchableEventManager::PrepareEvents() { assertChipStackLockedByCurrentThread(); - // Max out this duration and let CHIP set it appropriately. - mNextTimeout.tv_sec = DEFAULT_MIN_SLEEP_PERIOD; - mNextTimeout.tv_usec = 0; - PrepareEventsWithTimeout(mNextTimeout); -} - -void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeout) -{ + constexpr Clock::MonotonicMilliseconds kMaxTimeout = + static_cast(DEFAULT_MIN_SLEEP_PERIOD) * kMillisecondsPerSecond; const Clock::MonotonicMilliseconds currentTime = Clock::GetMonotonicMilliseconds(); - Clock::MonotonicMilliseconds awakenTime = currentTime + TimevalToMilliseconds(nextTimeout); + Clock::MonotonicMilliseconds awakenTime = currentTime + kMaxTimeout; Timer * timer = mTimerList.Earliest(); if (timer && Clock::IsEarlier(timer->mAwakenTime, awakenTime)) @@ -312,10 +307,10 @@ void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeou } const Clock::MonotonicMilliseconds sleepTime = (awakenTime > currentTime) ? (awakenTime - currentTime) : 0; - MillisecondsToTimeval(sleepTime, nextTimeout); + MillisecondsToTimeval(sleepTime, mNextTimeout); #if CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__ && !__MBED__ - chip::Mdns::GetMdnsTimeout(nextTimeout); + chip::Mdns::GetMdnsTimeout(mNextTimeout); #endif // CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__ mSelected = mRequest; diff --git a/src/system/WatchableEventManagerSelect.h b/src/system/WatchableEventManagerSelect.h index f3abf4161b3401..57cd1eeca1e77d 100644 --- a/src/system/WatchableEventManagerSelect.h +++ b/src/system/WatchableEventManagerSelect.h @@ -66,9 +66,6 @@ class WatchableEventManager void HandleEvents(); void EventLoopEnds() {} - // TODO(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer. - void PrepareEventsWithTimeout(timeval & nextTimeout); - static SocketEvents SocketEventsFromFDs(int socket, const fd_set & readfds, const fd_set & writefds, const fd_set & exceptfds); #if CHIP_SYSTEM_CONFIG_USE_DISPATCH diff --git a/src/system/tests/TestSystemTimer.cpp b/src/system/tests/TestSystemTimer.cpp index 9d7e7c6eff5a24..bb3e68d07b943b 100644 --- a/src/system/tests/TestSystemTimer.cpp +++ b/src/system/tests/TestSystemTimer.cpp @@ -50,10 +50,10 @@ using chip::ErrorStr; using namespace chip::System; -static void ServiceEvents(Layer & aLayer, ::timeval & aSleepTime) +static void ServiceEvents(Layer & aLayer) { #if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK - aLayer.WatchableEventsManager().PrepareEventsWithTimeout(aSleepTime); + aLayer.WatchableEventsManager().PrepareEvents(); aLayer.WatchableEventsManager().WaitForEvents(); aLayer.WatchableEventsManager().HandleEvents(); #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK @@ -61,8 +61,6 @@ static void ServiceEvents(Layer & aLayer, ::timeval & aSleepTime) #if CHIP_SYSTEM_CONFIG_USE_LWIP if (aLayer.State() == kLayerState_Initialized) { - // TODO: Currently timers are delayed by aSleepTime above. A improved solution would have a mechanism to reduce - // aSleepTime according to the next timer. aLayer.WatchableEventsManager().HandlePlatformTimer(); } #endif // CHIP_SYSTEM_CONFIG_USE_LWIP @@ -138,10 +136,7 @@ static void CheckOverflow(nlTestSuite * inSuite, void * aContext) while (!sOverflowTestDone) { - struct timeval sleepTime; - sleepTime.tv_sec = 0; - sleepTime.tv_usec = 1000; // 1 ms tick - ServiceEvents(lSys, sleepTime); + ServiceEvents(lSys); } lSys.CancelTimer(HandleTimerFailed, aContext); @@ -168,13 +163,10 @@ static void CheckStarvation(nlTestSuite * inSuite, void * aContext) { TestContext & lContext = *static_cast(aContext); Layer & lSys = *lContext.mLayer; - struct timeval sleepTime; lSys.StartTimer(0, HandleGreedyTimer, aContext); - sleepTime.tv_sec = 0; - sleepTime.tv_usec = 1000; // 1 ms tick - ServiceEvents(lSys, sleepTime); + ServiceEvents(lSys); } // Test Suite diff --git a/src/transport/raw/tests/NetworkTestHelpers.cpp b/src/transport/raw/tests/NetworkTestHelpers.cpp index a7f676cec25348..3040089da7e332 100644 --- a/src/transport/raw/tests/NetworkTestHelpers.cpp +++ b/src/transport/raw/tests/NetworkTestHelpers.cpp @@ -56,11 +56,8 @@ CHIP_ERROR IOContext::Shutdown() void IOContext::DriveIO() { // Set the select timeout to 100ms - struct timeval aSleepTime; - aSleepTime.tv_sec = 0; - aSleepTime.tv_usec = 100 * 1000; - - ServiceEvents(aSleepTime); + constexpr uint32_t kSleepTimeMilliseconds = 100; + ServiceEvents(kSleepTimeMilliseconds); } void IOContext::DriveIOUntil(unsigned maxWaitMs, std::function completionFunction)