Skip to content

Commit

Permalink
Fix timer cancellation for in progress timers
Browse files Browse the repository at this point in the history
  • Loading branch information
@andy31415
andy31415 committed Sep 2, 2022
1 parent 614e00a commit ca63e43
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 14 deletions.
25 changes: 17 additions & 8 deletions src/controller/tests/data_model/TestRead.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ bool expectedAttribute1 = true;
int16_t expectedAttribute2 = 42;
uint64_t expectedAttribute3 = 0xdeadbeef0000cafe;
uint8_t expectedAttribute4[256] = {
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf,
};

enum ResponseDirective
Expand Down Expand Up @@ -3004,6 +3004,9 @@ void TestReadInteraction::TestReadHandler_KillOverQuotaSubscriptions(nlTestSuite
// We have exactly one subscription than uses more resources than others, so the interaction model must evict it first, and we
// will have exactly kExpectedParallelPaths only when that subscription have been evicted. We use this indirect method to verify
// the subscriptions since the read client won't shutdown until the timeout fired.

// 6 over 18
ChipLogError(Test, "ATTR COUNT: %u over %u", (unsigned) readCallback.mAttributeCount, (unsigned) kExpectedParallelPaths);
NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == kExpectedParallelPaths);
NL_TEST_ASSERT(apSuite,
app::InteractionModelEngine::GetInstance()->GetNumActiveReadHandlers(
Expand Down Expand Up @@ -3059,10 +3062,16 @@ void TestReadInteraction::TestReadHandler_KillOverQuotaSubscriptions(nlTestSuite
[&]() { return app::InteractionModelEngine::GetInstance()->GetNumDirtySubscriptions() == 0; });

// Some subscriptions on fabric 1 should be evicted since fabric 1 is using more resources than the limits.
ChipLogError(Test, "ATTR COUNT: %u over %u", (unsigned) readCallback.mAttributeCount,
(unsigned) (app::InteractionModelEngine::kMinSupportedPathsPerSubscription *
app::InteractionModelEngine::kMinSupportedSubscriptionsPerFabric));
NL_TEST_ASSERT(apSuite,
readCallback.mAttributeCount ==
app::InteractionModelEngine::kMinSupportedPathsPerSubscription *
app::InteractionModelEngine::kMinSupportedSubscriptionsPerFabric);
ChipLogError(Test, "ATTR COUNT: %u over %u", (unsigned) readCallbackFabric2.mAttributeCount,
(unsigned) (app::InteractionModelEngine::kMinSupportedPathsPerSubscription *
app::InteractionModelEngine::kMinSupportedSubscriptionsPerFabric));
NL_TEST_ASSERT(apSuite,
readCallbackFabric2.mAttributeCount ==
app::InteractionModelEngine::kMinSupportedPathsPerSubscription *
Expand Down
12 changes: 9 additions & 3 deletions src/system/SystemLayerImplSelect.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appSt
VerifyOrReturn(mLayerState.IsInitialized());

TimerList::Node * timer = mTimerList.Remove(onComplete, appState);

if (timer == nullptr)
{
// Check if the timer is maybe currently being processed
timer = mExpiredTimersBeingProcessed.Remove(onComplete, appState);
}
VerifyOrReturn(timer != nullptr);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
Expand Down Expand Up @@ -469,9 +475,9 @@ void LayerImplSelect::HandleEvents()

// Obtain the list of currently expired timers. Any new timers added by timer callback are NOT handled on this pass,
// since that could result in infinite handling of new timers blocking any other progress.
TimerList expiredTimers = mTimerList.ExtractEarlier(Clock::Timeout(1) + SystemClock().GetMonotonicTimestamp());
TimerList::Node * timer = nullptr;
while ((timer = expiredTimers.PopEarliest()) != nullptr)
mExpiredTimersBeingProcessed = mTimerList.ExtractEarlier(Clock::Timeout(1) + SystemClock().GetMonotonicTimestamp());
TimerList::Node * timer = nullptr;
while ((timer = mExpiredTimersBeingProcessed.PopEarliest()) != nullptr)
{
mTimerPool.Invoke(timer);
}
Expand Down
16 changes: 13 additions & 3 deletions src/system/SystemLayerImplSelect.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,22 @@ class LayerImplSelect : public LayerSocketsLoop
void EventLoopEnds() override {}

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
void SetDispatchQueue(dispatch_queue_t dispatchQueue) override { mDispatchQueue = dispatchQueue; };
dispatch_queue_t GetDispatchQueue() override { return mDispatchQueue; };
void SetDispatchQueue(dispatch_queue_t dispatchQueue) override
{
mDispatchQueue = dispatchQueue;
};
dispatch_queue_t GetDispatchQueue() override
{
return mDispatchQueue;
};
void HandleTimerComplete(TimerList::Node * timer);
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

// Expose the result of WaitForEvents() for non-blocking socket implementations.
bool IsSelectResultValid() const { return mSelectResult >= 0; }
bool IsSelectResultValid() const
{
return mSelectResult >= 0;
}

protected:
static SocketEvents SocketEventsFromFDs(int socket, const fd_set & readfds, const fd_set & writefds, const fd_set & exceptfds);
Expand All @@ -101,6 +110,7 @@ class LayerImplSelect : public LayerSocketsLoop

TimerPool<TimerList::Node> mTimerPool;
TimerList mTimerList;
TimerList mExpiredTimersBeingProcessed; // timers handled by HandleEvents
timeval mNextTimeout;

// Members for select loop
Expand Down

0 comments on commit ca63e43

Please sign in to comment.