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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ICD] Server side subscription persistence and resumption #24361
[ICD] Server side subscription persistence and resumption #24361
Changes from 41 commits
7f8db2f
0e003ba
a3b8add
c68cf85
c6fb866
a82a9d0
57cf32e
05006c3
9f56e48
02ebc9c
65b17af
6bacbbd
865a169
3d35c8d
1d5bed2
c050d5b
843dfb9
e442d5a
2316be4
cd757e1
bda0ca9
8cc51ef
3660392
8b6fe6a
dd6903d
cd69518
3a509f4
6960898
194361e
f8d463b
3506edf
122224c
22cb37c
2445ab5
72e95cd
0a26d5a
952007f
866ba53
cedd227
b11ca7e
29cbb72
4e121e2
b8ef2e6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably error out if we have some read handlers allocated already, because that would mean we might create read handlers to represent stored subscriptions that are already represented by an allocated read handler.
And the declaration in the header should document that this is expected to be called during startup, after initing the IM engine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not using the IM engine for this here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably check that
mInteractionType == InteractionType::Subscribe
before using the otherwise-probably-not-meaningful mSubscriptionId.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetInitiatorNodeId()
andGetAccessingFabricIndex()
might not really be meaningful ifGetSession()
returns null (which for example it will if this gets closed from one of the error-path calls inResumeSubscription
.I think we should not use those methods here if
GetSession()
is null. Separately, should the error paths inResumeSubscription
clear out the stored subscription?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the model here is that the stored thing is cleaned up if we hit one of the non-timeout error cases in ReadHandler, right?
It feels like we should also clean it up in
InteractionModelEngine::OnReadInitialRequest
whenkeepExistingSubscriptions
is false and weReleaseObject
some read handlers to tear down those subscriptions, right?But we don't want to clean it up in
InteractionModelEngine::Shutdown
when we drop all the ReadHandlers, because at that point we want to keep the state stored. We should probably document that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method's definition and declaration should probably be conditional on
CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These HandleDevice* should also be conditioned on CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is going to not actually erase out stored subscription, since we have no session. I assume that is in fact the behavior we want here, but we might want to document that... and maybe pass the "don't erase" flag explicitly if that's what we really want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing, I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should document that this param is ignored for non-subscription ReadHandlers.