Fix timer cancellation to be reliable in LayerImplSelect. #22375
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
lazarkov,
LuDuda,
mlepage-google,
msandstedt,
mspang and
robszewczyk
|
It looks like something is going wrong with testSystemLayer on ESP32. Looking into that. |
bzbarsky-apple
force-pushed
the
fix-timer-cancellation
branch
from
05aba61
to
9dd99b1
Compare
|
PR #22375: Size comparison from ba8a495 to 9dd99b1 Increases (34 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (34 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
…c/system/tests/TestSystemScheduleWork.cpp history.
To minimize risk, the changes here keep the "grab all the timers we should fire, then fire them" setup instead of switching to the "fire the timers one at a time" approach LayerImplFreeRTOS uses. The fix consists of the following parts: 1) Store the list of timers to fire in a member, so we can cancel things from that list as needed. 2) To avoid canceling things scheduled by ScheduleWork, remove the CancelTimer call in ScheduleWork. This does mean we now allow multiple timers for the same callback+appState in the timer list, if they were created by ScheduleWork, but that should be OK, since the only reason that pair needs to be unique is to allow cancellation and we never want to cancel the things ScheduleWork schedules. As a followup we should stop using the timer list for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS does, but that involves fixing the unit tests that call ScheduleWork without actually running the platfor manager event loop and expect it to work somehow. TestRead was failing the sanity assert for not losing track of timers to fire, because it was spinning a nested event loop. The changes to that test stop it from doing that. Fixes project-chip#19387 Fixes project-chip#22160
bzbarsky-apple
force-pushed
the
fix-timer-cancellation
branch
from
9dd99b1
to
214afe6
Compare
|
PR #22375: Size comparison from 05172d2 to 214afe6 Increases (44 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (44 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
|
The fake platform's event loop does not actually process the SystemLayer events.
|
PR #22375: Size comparison from 05172d2 to 1a864c7 Increases (34 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (7 builds for cc13x2_26x2, esp32)
Full report (34 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
OK, fair enough. |
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Sep 8, 2022
…roject-chip#22375 This follows the rationale in project-chip#22375 about posting the same work (same callback and appstate) again should not cancel the previously scheduled instance. Implemented now for the CHIP_SYSTEM_CONFIG_USE_LIBEV=1 case as well.
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
…ip#22375) * Duplicate src/system/tests/TestSystemScheduleLambda.cpp history in src/system/tests/TestSystemScheduleWork.cpp history. * Fix timer cancellation to be reliable in LayerImplSelect. To minimize risk, the changes here keep the "grab all the timers we should fire, then fire them" setup instead of switching to the "fire the timers one at a time" approach LayerImplFreeRTOS uses. The fix consists of the following parts: 1) Store the list of timers to fire in a member, so we can cancel things from that list as needed. 2) To avoid canceling things scheduled by ScheduleWork, remove the CancelTimer call in ScheduleWork. This does mean we now allow multiple timers for the same callback+appState in the timer list, if they were created by ScheduleWork, but that should be OK, since the only reason that pair needs to be unique is to allow cancellation and we never want to cancel the things ScheduleWork schedules. As a followup we should stop using the timer list for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS does, but that involves fixing the unit tests that call ScheduleWork without actually running the platfor manager event loop and expect it to work somehow. TestRead was failing the sanity assert for not losing track of timers to fire, because it was spinning a nested event loop. The changes to that test stop it from doing that. Fixes project-chip#19387 Fixes project-chip#22160 * Add a unit test that timer cancelling works even for currently "in progress" timers * Address review comments. * Fix shadowing problem. * Turn off TestSystemScheduleWork on esp32 QEMU for now. * Turn off TestSystemScheduleWork on the fake platform too. The fake platform's event loop does not actually process the SystemLayer events. Co-authored-by: Andrei Litvin <andy314@gmail.com>
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Oct 8, 2022
This follows the rationale in project-chip#22375 about posting the same work (same callback and appstate) again should not cancel the previously scheduled instance.
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Oct 28, 2022
This follows the rationale in project-chip#22375 about posting the same work (same callback and appstate) again should not cancel the previously scheduled instance.
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Dec 29, 2022
This follows the rationale in project-chip#22375 about posting the same work (same callback and appstate) again should not cancel the previously scheduled instance.
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Apr 6, 2023
This follows the rationale in project-chip#22375 about posting the same work (same callback and appstate) again should not cancel the previously scheduled instance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To minimize risk, the changes here keep the "grab all the timers we
should fire, then fire them" setup instead of switching to the "fire
the timers one at a time" approach LayerImplFreeRTOS uses.
The fix consists of the following parts:
that list as needed.
call in ScheduleWork. This does mean we now allow multiple timers for the
same callback+appState in the timer list, if they were created by
ScheduleWork, but that should be OK, since the only reason that pair needs to
be unique is to allow cancellation and we never want to cancel the things
ScheduleWork schedules. As a followup we should stop using the timer list
for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS
does, but that involves fixing the unit tests that call ScheduleWork without
actually running the platfor manager event loop and expect it to work
somehow.
TestRead was failing the sanity assert for not losing track of timers
to fire, because it was spinning a nested event loop. The changes to
that test stop it from doing that.
Fixes #19387
Fixes #22160
Problem
Timer cancellation sometimes fails, leading to crashes.
Change overview
Fix timer cancellation to be reliable.
Testing
New unit tests added for canceling timers and ScheduleWork not canceling a previous ScheduleWork.