Skip to content

Commit

Permalink
Mark CASE sessions as defunct when we don't get expected responses/ac…
Browse files Browse the repository at this point in the history
…ks. (#20820)

* Mark CASE sessions as defunct when we don't get expected responses/acks.

* Address review comments.

* Don't mark PASE sessions defunct.

* Add automated test
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Sep 1, 2023
1 parent 0331547 commit 1236825
Show file tree
Hide file tree
Showing 14 changed files with 1,771 additions and 606 deletions.
5 changes: 4 additions & 1 deletion examples/chip-tool/commands/tests/TestCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ CHIP_ERROR TestCommand::WaitForCommissionee(const char * identity,
// or is just starting out fresh outright. Let's make sure we're not re-using any cached CASE sessions
// that will now be stale and mismatched with the peer, causing subsequent interactions to fail.
//
GetCommissioner(identity).SessionMgr()->ExpireAllSessions(chip::ScopedNodeId(value.nodeId, fabricIndex));
if (value.expireExistingSession.ValueOr(true))
{
GetCommissioner(identity).SessionMgr()->ExpireAllSessions(chip::ScopedNodeId(value.nodeId, fabricIndex));
}

SetIdentity(identity);
return GetCommissioner(identity).GetConnectedDevice(value.nodeId, &mOnDeviceConnectedCallback,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,11 @@ class TestCommandBridge : public CHIPCommandBridge,
// Invalidate our existing CASE session; otherwise getConnectedDevice
// will just hand it right back to us without establishing a new CASE
// session when a reboot is done on the server.
if (GetDevice(identity) != nil) {
[GetDevice(identity) invalidateCASESession];
mConnectedDevices[identity] = nil;
if (value.expireExistingSession.ValueOr(true)) {
if (GetDevice(identity) != nil) {
[GetDevice(identity) invalidateCASESession];
mConnectedDevices[identity] = nil;
}
}

[controller getBaseDevice:value.nodeId
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ class {{filename}}: public TestCommandBridge

{{#chip_tests_item_responses}}
{{#if error}}
VerifyOrReturn(CheckValue("status", err ? err.code : 0, {{error}}));
VerifyOrReturn(CheckValue("status", err ? ([err.domain isEqualToString:MTRInteractionErrorDomain] ? err.code : EMBER_ZCL_STATUS_FAILURE) : 0, {{error}}));
NextTest();
{{else}}
VerifyOrReturn(CheckValue("status", err ? err.code : 0, 0));
Expand Down
2 changes: 1 addition & 1 deletion src/app/OperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ void OperationalDeviceProxy::OnFirstMessageDeliveryFailed()

void OperationalDeviceProxy::OnSessionHang()
{
// TODO: establish a new session
Disconnect();
}

void OperationalDeviceProxy::ShutdownSubscriptions()
Expand Down
75 changes: 75 additions & 0 deletions src/app/tests/suites/TestCASERecovery.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# Copyright (c) 2022 Project CHIP Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

name: Test for CASE recovery

config:
nodeId: 0x12344321
cluster: "Basic"
endpoint: 0

tests:
- label: "Wait for the commissioned device to be retrieved"
cluster: "DelayCommands"
command: "WaitForCommissionee"
arguments:
values:
- name: "nodeId"
value: nodeId

# Make sure we have a working CASE session.
- label: "Read an attribute"
command: "readAttribute"
attribute: "DataModelRevision"
response:
value: 1

- label: "Reboot the server"
cluster: "SystemCommands"
command: "Reboot"

- label: "Re-get our session, but without expiring sesssions"
cluster: "DelayCommands"
command: "WaitForCommissionee"
arguments:
values:
- name: "nodeId"
value: nodeId
- name: "expireExistingSession"
value: false

# Try to do something with the CASE session. This should time out, but
# mark the CASE session as defunct.
- label: "Read an attribute again"
command: "readAttribute"
attribute: "DataModelRevision"
response:
error: FAILURE

- label: "Re-get our session, but without expiring sesssions"
cluster: "DelayCommands"
command: "WaitForCommissionee"
arguments:
values:
- name: "nodeId"
value: nodeId
- name: "expireExistingSession"
value: false

# Now trying to do another read should create a new CASE session successfully.
- label: "Read an attribute a third time"
command: "readAttribute"
attribute: "DataModelRevision"
response:
value: 1
1 change: 1 addition & 0 deletions src/app/tests/suites/tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,7 @@ function getTests() {
];

const Others = [
"TestCASERecovery",
"TestCluster",
"TestClusterComplexTypes",
"TestConstraints",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const WaitForCommissioning = {

const WaitForCommissionee = {
name : 'WaitForCommissionee',
arguments : [ { type : 'NODE_ID', name : 'nodeId' } ],
arguments : [ { type : 'NODE_ID', name : 'nodeId' }, { type : 'BOOLEAN', name : 'expireExistingSession', isOptional : true } ],
};

const WaitForMessage = {
Expand Down
11 changes: 11 additions & 0 deletions src/messaging/ExchangeContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,17 @@ void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
{
SetResponseExpected(false);

// mSession might be null if this timeout is due to the session being
// evicted.
if (mSession)
{
if (mSession->IsSecureSession() && mSession->AsSecureSession()->IsCASESession())
{
mSession->AsSecureSession()->MarkAsDefunct();
}
mSession->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
}

ExchangeDelegate * delegate = GetDelegate();

// Call the user's timeout handler.
Expand Down
18 changes: 9 additions & 9 deletions src/messaging/ExchangeContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,15 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
// SecureSession.
void AbortAllOtherCommunicationOnFabric();

/**
* Determine whether a response is currently expected for a message that was sent over
* this exchange. While this is true, attempts to send other messages that expect a response
* will fail.
*
* @return Returns 'true' if response expected, else 'false'.
*/
bool IsResponseExpected() const;

private:
class ExchangeSessionHolder : public SessionHolderWithDelegate
{
Expand All @@ -212,15 +221,6 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
ExchangeSessionHolder mSession; // The connection state
uint16_t mExchangeId; // Assigned exchange ID.

/**
* Determine whether a response is currently expected for a message that was sent over
* this exchange. While this is true, attempts to send other messages that expect a response
* will fail.
*
* @return Returns 'true' if response expected, else 'false'.
*/
bool IsResponseExpected() const;

/**
* Determine whether we are expecting our consumer to send a message on
* this exchange (i.e. WillSendMessage was called and the message has not
Expand Down
14 changes: 13 additions & 1 deletion src/messaging/ReliableMessageMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,19 @@ void ReliableMessageMgr::ExecuteActions()

// Don't check whether the session in the exchange is valid, because when the session is released, the retrans entry is
// cleared inside ExchangeContext::OnSessionReleased, so the session must be valid if the entry exists.
entry->ec->GetSessionHandle()->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
SessionHandle session = entry->ec->GetSessionHandle();

// If the exchange is expecting a response, it will handle sending
// this notification once it detects that it has not gotten a
// response. Otherwise, we need to do it.
if (!entry->ec->IsResponseExpected())
{
if (session->IsSecureSession() && session->AsSecureSession()->IsCASESession())
{
session->AsSecureSession()->MarkAsDefunct();
}
session->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
}

// Do not StartTimer, we will schedule the timer at the end of the timer handler.
mRetransTable.ReleaseObject(entry);
Expand Down
8 changes: 6 additions & 2 deletions src/transport/Session.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,13 @@ class Session

void DispatchSessionEvent(SessionDelegate::Event event)
{
for (auto & holder : mHolders)
// Holders might remove themselves when notified.
auto holder = mHolders.begin();
while (holder != mHolders.end())
{
holder.DispatchSessionEvent(event);
auto cur = holder;
++holder;
cur->DispatchSessionEvent(event);
}
}

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 1236825

Please sign in to comment.