Fix CASE Churn caused by subscriptions on tv #23616
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
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13 and
harsha-rajendran
|
PR #23616: Size comparison from 6519b91 to a3e60cf Increases (21 builds for cc13x2_26x2, cyw30739, efr32, nrfconnect, psoc6)
Decreases (10 builds for cc13x2_26x2, psoc6)
Full report (22 builds for cc13x2_26x2, cyw30739, efr32, mbed, nrfconnect, psoc6)
|
sharadb-amazon
approved these changes
Nov 15, 2022
andy31415
reviewed
Nov 15, 2022
examples/tv-casting-app/tv-casting-common/include/MediaSubscriptionBase.h
Outdated
Show resolved
Hide resolved
bzbarsky-apple
previously requested changes
Nov 15, 2022
|
PR #23616: Size comparison from 6519b91 to 02a202e Increases (6 builds for bl602, bl702, nrfconnect)
Decreases (2 builds for bl702)
Full report (9 builds for bl602, bl702, linux, mbed, nrfconnect)
|
…nd controller are enabled
bzbarsky-apple
dismissed
their stale review
Removing changes requested so this is not blocked while I am on vacation
andy31415
approved these changes
Nov 21, 2022
mrjerryjohns
suggested changes
Nov 22, 2022
There was a problem hiding this comment.
Our stack is not at all built to deal with multiple instances of canonical singletons like ExchangeMgr, and a patch like this gives the semblance of fixing the problem, when in fact, the problem is much deeper.
ReadHandler adopting the ExchangeMgr alone is insufficient, since there are plenty of complex interactions that go through InteractionModelEngine and the ReportingEngine that results in them accessing again the ExchangeMgr instance behind the InteractionModelEngine.
The correct fix is one of two possibilities:
- Fix the stack to do away with the singleton nature of these entities, and permit true multi-instancing of the core state of the stack (not at all trivial).
- Live with the current constraints, and to achieve dual server/client modality, you dont need to instantiate multiple instances of these singletons. That is the approach most people have adopted (including us at Google).
mrjerryjohns
approved these changes
Nov 24, 2022
lazarkov
approved these changes
Nov 24, 2022
sharadb-amazon
pushed a commit
to sharadb-amazon/connectedhomeip
that referenced
this pull request
Nov 28, 2022
* Fix CASE Churn caused by subscriptions on tv * Address comments * User feature flag for IM fix since it only applies when both server and controller are enabled * Address feedback * fix build
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.
Fixes #23618 : CASE session churn observed when tv-casting-app creates subscriptions on tv-app.
The fix here is to grab a reference to the ExchangeMgr used to create the subscription's ReadHandler and use that one to send the ReportData
The fix here is to set this optional param to true.