Add support for re-using CASE sessions #17266
Merged
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
mrjerryjohns
changed the title
pullapprove
bot
requested review from
andy31415,
anush-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
kpschoedel,
lazarkov and
LuDuda
|
PR #17266: Size comparison from 97b144a to df6dacd Increases (14 builds for cc13x2_26x2, cyw30739, efr32, esp32, linux, nrfconnect, p6, telink)
Decreases (8 builds for cyw30739, efr32, k32w, mbed, p6)
Full report (22 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
msandstedt
reviewed
Apr 11, 2022
msandstedt
approved these changes
Apr 11, 2022
andy31415
reviewed
Apr 12, 2022
andy31415
reviewed
Apr 12, 2022
bzbarsky-apple
previously requested changes
Apr 13, 2022
mrjerryjohns
force-pushed
the
reuse-case
branch
from
b8361bd
to
8965367
Compare
|
PR #17266: Size comparison from 964adbc to 8965367 Increases (24 builds for cc13x2_26x2, cyw30739, efr32, esp32, linux, nrfconnect, p6, telink)
Decreases (10 builds for cc13x2_26x2, cyw30739, efr32, k32w, mbed, p6)
Full report (32 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
carol-apple
approved these changes
Apr 15, 2022
msandstedt
reviewed
Apr 15, 2022
msandstedt
approved these changes
Apr 15, 2022
chrisdecenzo
approved these changes
Apr 15, 2022
mrjerryjohns
dismissed
bzbarsky-apple’s stale review
Chatted with Boris, he's out today. Will merge, and if he has any issues, he'll follow-up next week with issues.
| } | ||
|
|
||
| // | ||
| // Do not touch this instance anymore; it might have been destroyed by a |
There was a problem hiding this comment.
"device", not "this": this is a static method.
| @@ -144,6 +144,16 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr | |||
| */ | |||
| virtual CHIP_ERROR Shutdown(); | |||
|
|
|||
| SessionManager * SessionMgr() | |||
There was a problem hiding this comment.
I think we should strongly consider removing this and changing the one consumer we have (chip-tool) to get the system state from the device controller factory and get the session manager from there...
Failing that, a name that more clearly indicates that null can be returned (e.g. GetSessionMgr), or a signature that can fail, would be good...
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.
Problem
CASE sessions that were setup as a side-effect of a node acting as a responder (i.e sessions setup by
CASEServer) were not getting re-used correctly by client-side logic (i.eCASEClient), see #15986.This was further exacerbated by calls to expire all sessions upon completion of CASE session establishment, which meant a continuous ping-pong effect of a previously established session being destroyed upon establishment of a new session on each side, but would tear down the wrong one on the other.
Change Overview
SessionManageris the source of truth for all active sessions, so extended the existingFindSecureSessionForNodeto take a session type as an argument to ensure we only find active CASE sessions, and calling this fromOperationalDeviceProxy.Other changes include:
DequeueConnectionCallbackvariants for failure and success into a single method that does both since it was always mutually exclusive.Connectby always just enqueuing, and then dequeuing based on failure/success.SetConnectedSessionmethod that should never have be a public API, and fixing the tv-app (which is the only consumer).Testing
Repro'ed the original issue by using the REPL and a
ota-provider-appinstance to setup a CASE session to the provider, and then sending it aAnnounceCommandindicating the REPL is the provider.I validated that the changes here correctly resulted in the app re-using the CASE session to initiate the query.
I also validated that subsequent tear-down of the REPL, restart and re-doing the above flow resulted in the query image using the newly created session.