Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
SystemLayerImplSelect: libev: avoid timers firing early (#28434) (#28740
Browse files Browse the repository at this point in the history
)

* SystemLayerImplSelect: libev: avoid timers firing slightly too early (#28434)

This fixes a ReportEngine problem that was caused by libev based timers
firing slightly (in the range of 20mS) too early, because libev by
default uses the time when events started processing as the "now"
reference for relative timers for efficiency reasons.

To ensure timers cannot fire early, timer setup must compensate for
any difference between ev_now() which is the time events started
processing and ev_time(), which is the actual current time (but is
a bit less efficient to obtain).

# Conflicts:
#	src/system/SystemLayerImplSelect.cpp

* SystemLayerImplSelect: libev: eliminate I/O wakeup thread

libev based regular builds do not need a separate I/O wakeup thread, so
the wakeup thread can be eliminated in CHIP_SYSTEM_CONFIG_USE_LIBEV case.

In normal operation, libev based builds also never call Signal(), so if it
is still called it now emits a error log message, to indicate something might
be wrong in the setup.

However we keep Signal() and related LayerSocketsLoop methods, following the
Darwin dispatch implementation, as fallback to select-based event handling
seems to be needed for some I/O tests.

As noted in a comment to the original libev PR [1], eventually, the select() based
mainloop and external mainloop based solutions like Darwin Dispatch and libev
should be detangled and extracted into separate classes, adapting all the tests
that somehow rely on the select() fallback. As a single self-funded
developer however, I cannot possibly be expected to solve this for Apple ;-)

[1] #24232 (review)

* SystemLayerImplSelect: reword comment: early firing timers can happen

- the fix prevents them in normal libev case
- however the caller MUST NOT rely on timers *never* firing a bit early
plan44 authored and pull[bot] committed Dec 15, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 1702c68 commit 93ec12f
Showing 4 changed files with 25 additions and 5 deletions.
20 changes: 19 additions & 1 deletion src/system/SystemLayerImplSelect.cpp
Original file line number Diff line number Diff line change
@@ -66,8 +66,10 @@ CHIP_ERROR LayerImplSelect::Init()
mHandleSelectThread = PTHREAD_NULL;
#endif // CHIP_SYSTEM_CONFIG_POSIX_LOCKING

#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
// Create an event to allow an arbitrary thread to wake the thread in the select loop.
ReturnErrorOnFailure(mWakeEvent.Open(*this));
#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV

VerifyOrReturnError(mLayerState.SetInitialized(), CHIP_ERROR_INCORRECT_STATE);
return CHIP_NO_ERROR;
@@ -113,13 +115,18 @@ void LayerImplSelect::Shutdown()
mTimerPool.ReleaseAll();
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV

#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
mWakeEvent.Close(*this);
#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV

mLayerState.ResetFromShuttingDown(); // Return to uninitialized state to permit re-initialization.
}

void LayerImplSelect::Signal()
{
#if CHIP_SYSTEM_CONFIG_USE_LIBEV
ChipLogError(DeviceLayer, "Signal() should not be called in CHIP_SYSTEM_CONFIG_USE_LIBEV builds (might be ok in tests)");
#else
/*
* Wake up the I/O thread by writing a single byte to the wake pipe.
*
@@ -143,6 +150,7 @@ void LayerImplSelect::Signal()

ChipLogError(chipSystemLayer, "System wake event notify failed: %" CHIP_ERROR_FORMAT, status.Format());
}
#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV
}

CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState)
@@ -184,7 +192,14 @@ CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallba
ev_timer_init(&timer->mLibEvTimer, &LayerImplSelect::HandleLibEvTimer, 1, 0);
timer->mLibEvTimer.data = timer;
auto t = Clock::Milliseconds64(delay).count();
ev_timer_set(&timer->mLibEvTimer, static_cast<double>(t) / 1E3, 0.);
// Note: libev uses the time when events started processing as the "now" reference for relative timers,
// for efficiency reasons. This point in time is represented by ev_now().
// The real time is represented by ev_time().
// Without correction, this leads to timers firing a bit too early relative to the time StartTimer()
// is called. So the relative value passed to ev_timer_set() is adjusted (increased) here.
// Note: Still, slightly early (and of course, late) firing timers are something the caller MUST be prepared for,
// because edge cases like system clock adjustments may cause them even with the correction applied here.
ev_timer_set(&timer->mLibEvTimer, (static_cast<double>(t) / 1E3) + ev_time() - ev_now(mLibEvLoopP), 0.);
(void) mTimerList.Add(timer);
ev_timer_start(mLibEvLoopP, &timer->mLibEvTimer);
return CHIP_NO_ERROR;
@@ -269,7 +284,10 @@ void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appSt
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV

mTimerPool.Release(timer);
#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
// LIBEV has no I/O wakeup thread, so must not call Signal()
Signal();
#endif
}

CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void * appState)
2 changes: 2 additions & 0 deletions src/system/SystemLayerImplSelect.h
Original file line number Diff line number Diff line change
@@ -146,7 +146,9 @@ class LayerImplSelect : public LayerSocketsLoop
int mSelectResult;

ObjectLifeCycle mLayerState;
#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
WakeEvent mWakeEvent;
#endif

#if CHIP_SYSTEM_CONFIG_POSIX_LOCKING
std::atomic<pthread_t> mHandleSelectThread;
4 changes: 2 additions & 2 deletions src/system/WakeEvent.cpp
Original file line number Diff line number Diff line change
@@ -23,7 +23,7 @@

#include <system/SystemConfig.h>

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
#if CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV

#include <system/WakeEvent.h>

@@ -207,4 +207,4 @@ CHIP_ERROR WakeEvent::Notify() const
} // namespace System
} // namespace chip

#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV
4 changes: 2 additions & 2 deletions src/system/WakeEvent.h
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@
// Include configuration headers
#include <system/SystemConfig.h>

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
#if CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV

#include <lib/core/CHIPError.h>
#include <system/SocketEvents.h>
@@ -67,4 +67,4 @@ class WakeEvent
} // namespace System
} // namespace chip

#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV

0 comments on commit 93ec12f

Please sign in to comment.