Fix session allocation loop when shutting down with an open commissioning window. #20715
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
…ning window. After project-chip#20487 if we shut down with a commissioning window open we end up in a loop where the session manager shutdown marks the tentative PASE session for eviction, we treat that as a commissioning error and start listening for PASE again, creating a new session, etc. With a heap pool this ends up happening to work in that we keep evicting the new sessions until we hit the 20-attempt limit and close the commissioning window. With a non-heap pool, I sort of wonder what happens, exactly. The fix here is in two parts, with either part enough on its own to fix the behavior described above: 1) Shut down the commissioning window manager earlier, before we shut down the session manager. And correspondingly move its initialization during server init later. 2) Once session manager starts shutdown, refuse to create any new sessions.
pullapprove
bot
requested review from
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,
kghost,
kpschoedel,
lazarkov,
LuDuda and
mlepage-google
pullapprove
bot
requested review from
msandstedt,
mspang,
robszewczyk,
saurabhst,
selissia,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
|
PR #20715: Size comparison from 37bb2df to 78c92ec Increases (21 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, telink)
Decreases (14 builds for bl602, cc13x2_26x2, efr32, p6)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
Damian-Nordic
approved these changes
Jul 14, 2022
andy31415
approved these changes
Jul 14, 2022
msandstedt
approved these changes
Jul 14, 2022
|
PR #20715: Size comparison from 37bb2df to 3df266b Increases (5 builds for esp32, mbed, nrfconnect)
Decreases (1 build for nrfconnect)
Full report (5 builds for esp32, mbed, nrfconnect)
|
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.
After #20487 if we shut down with a commissioning window open we end up in a
loop where the session manager shutdown marks the tentative PASE session for
eviction, we treat that as a commissioning error and start listening for PASE
again, creating a new session, etc. With a heap pool this ends up happening to
work in that we keep evicting the new sessions until we hit the 20-attempt limit
and close the commissioning window. With a non-heap pool, I sort of wonder what
happens, exactly.
The fix here is in two parts, with either part enough on its own to fix the
behavior described above:
Shut down the commissioning window manager earlier, before we shut down the
session manager. And correspondingly move its initialization during server
init later.
Once session manager starts shutdown, refuse to create any new sessions.
Problem
Stacks like this if I start all-clusters-app in uncommissioned state and then Ctrl-C it:
Change overview
See above.
Testing
See above about hitting Ctrl-C. With this PR there is no repeated listening for PASE, no
bits. Just clean shutdown.