Refactor exchange usage in IM to improve cirque stability (#6730)

yufengwangca · bzbarsky-apple · pull[bot] · commit 362591701f02 · 2021-06-14T13:52:01.000Z
* Refactor exhange usage in IM to improve cirque stability

* Update src/app/ReadClient.cpp

Co-authored-by: Boris Zbarsky &lt;bzbarsky@apple.com&gt;

* Update src/app/CommandSender.cpp

Co-authored-by: Boris Zbarsky &lt;bzbarsky@apple.com&gt;

* Rename ClearExistingExchangeContext to AbortExistingExchangeContext

Co-authored-by: Boris Zbarsky &lt;bzbarsky@apple.com&gt;
diff --git a/scripts/tests/cirque_tests.sh b/scripts/tests/cirque_tests.sh
@@ -36,7 +36,7 @@ OT_SIMULATION_CACHE="$CIRQUE_CACHE_PATH/ot-simulation.tgz"
 CIRQUE_TESTS=(
     "EchoTest"
     "MobileDeviceTest"
-    "InteractionModel"
+    "InteractionModelTest"
 )
 
 BOLD_GREEN_TEXT="\033[1;32m"
diff --git a/src/app/Command.cpp b/src/app/Command.cpp
@@ -37,7 +37,6 @@ CHIP_ERROR Command::Init(Messaging::ExchangeManager * apExchangeMgr, Interaction
     // Error if already initialized.
     VerifyOrExit(apExchangeMgr != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(mpExchangeMgr == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrExit(mpExchangeCtx == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
 
     mpExchangeMgr = apExchangeMgr;
     mpDelegate    = apDelegate;
@@ -53,7 +52,7 @@ CHIP_ERROR Command::Reset()
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     CommandList::Builder commandListBuilder;
-    ClearExistingExchangeContext();
+    AbortExistingExchangeContext();
 
     if (mCommandMessageBuf.IsNull())
     {
@@ -133,7 +132,7 @@ void Command::Shutdown()
     mCommandMessageWriter.Reset();
     mCommandMessageBuf = nullptr;
 
-    ClearExistingExchangeContext();
+    AbortExistingExchangeContext();
 
     mpExchangeMgr = nullptr;
     mpDelegate    = nullptr;
@@ -213,7 +212,7 @@ CHIP_ERROR Command::ConstructCommandPath(const CommandPathParams & aCommandPathP
     return commandPath.GetError();
 }
 
-CHIP_ERROR Command::ClearExistingExchangeContext()
+CHIP_ERROR Command::AbortExistingExchangeContext()
 {
     // Discard any existing exchange context. Effectively we can only have one Echo exchange with
     // a single node at any one time.
diff --git a/src/app/Command.h b/src/app/Command.h
@@ -119,7 +119,7 @@ class Command
     virtual CHIP_ERROR ProcessCommandDataElement(CommandDataElement::Parser & aCommandElement) = 0;
 
 protected:
-    CHIP_ERROR ClearExistingExchangeContext();
+    CHIP_ERROR AbortExistingExchangeContext();
     void MoveToState(const CommandState aTargetState);
     CHIP_ERROR ProcessCommandMessage(System::PacketBufferHandle && payload, CommandRoleId aCommandRoleId);
     CHIP_ERROR ConstructCommandPath(const CommandPathParams & aCommandPathParams, CommandDataElement::Builder aCommandDataElement);
diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp
@@ -41,7 +41,9 @@ CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId, Transport::AdminId
     err = FinalizeCommandsMessage();
     SuccessOrExit(err);
 
-    ClearExistingExchangeContext();
+    // Discard any existing exchange context. Effectively we can only have one exchange per CommandSender
+    // at any one time.
+    AbortExistingExchangeContext();
 
     // Create a new exchange context.
     // TODO: temprary create a SecureSessionHandle from node id, will be fix in PR 3602
@@ -59,7 +61,7 @@ CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId, Transport::AdminId
 exit:
     if (err != CHIP_NO_ERROR)
     {
-        ClearExistingExchangeContext();
+        AbortExistingExchangeContext();
     }
     ChipLogFunctError(err);
 
@@ -70,27 +72,23 @@ void CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExchangeCon
                                       const PayloadHeader & aPayloadHeader, System::PacketBufferHandle aPayload)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
-    // Assert that the exchange context matches the client's current context.
-    // This should never fail because even if SendCommandRequest is called
-    // back-to-back, the second call will call Close() on the first exchange,
-    // which clears the OnMessageReceived callback.
 
-    VerifyOrDie(apExchangeContext == mpExchangeCtx);
-
-    // Verify that the message is an Invoke Command Response.
-    // If not, close the exchange and free the payload.
-    if (!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandResponse))
-    {
-        apExchangeContext->Close();
-        mpExchangeCtx = nullptr;
-        goto exit;
-    }
+    VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrExit(aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandResponse),
+                 err = CHIP_ERROR_INVALID_MESSAGE_TYPE);
 
     err = ProcessCommandMessage(std::move(aPayload), CommandRoleId::SenderId);
     SuccessOrExit(err);
 
 exit:
+    ChipLogFunctError(err);
+
+    // Close the exchange cleanly so that the ExchangeManager will send an ack for the message we just received.
+    // This needs to be done before the Reset() call, because Reset() aborts mpExchangeCtx if its not null.
+    mpExchangeCtx->Close();
+    mpExchangeCtx = nullptr;
     Reset();
+
     if (mpDelegate != nullptr)
     {
         if (err != CHIP_NO_ERROR)
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
@@ -34,21 +34,21 @@ CHIP_ERROR ReadClient::Init(Messaging::ExchangeManager * apExchangeMgr, Interact
     // Error if already initialized.
     VerifyOrExit(apExchangeMgr != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(mpExchangeMgr == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrExit(mpExchangeCtx == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
 
     mpExchangeMgr = apExchangeMgr;
-    mpExchangeCtx = nullptr;
     mpDelegate    = apDelegate;
     mState        = ClientState::Initialized;
 
+    AbortExistingExchangeContext();
+
 exit:
     ChipLogFunctError(err);
     return err;
 }
 
 void ReadClient::Shutdown()
 {
-    ClearExistingExchangeContext();
+    AbortExistingExchangeContext();
     mpExchangeMgr = nullptr;
     mpDelegate    = nullptr;
     MoveToState(ClientState::Uninitialized);
@@ -88,7 +88,10 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, Transport::AdminId aAdmin
                   InteractionModelEngine::GetInstance()->GetReadClientArrayIndex(this), GetStateStr());
     VerifyOrExit(ClientState::Initialized == mState, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(mpDelegate != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrExit(mpExchangeCtx == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
+
+    // Discard any existing exchange context. Effectively we can only have one exchange per ReadClient
+    // at any one time.
+    AbortExistingExchangeContext();
 
     {
         System::PacketBufferTLVWriter writer;
@@ -175,23 +178,33 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, Transport::AdminId aAdmin
 
 exit:
     ChipLogFunctError(err);
+
+    if (err != CHIP_NO_ERROR)
+    {
+        AbortExistingExchangeContext();
+    }
+
     return err;
 }
 
 void ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
                                    const PayloadHeader & aPayloadHeader, System::PacketBufferHandle aPayload)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
+
+    VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData),
                  err = CHIP_ERROR_INVALID_MESSAGE_TYPE);
-    VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
     err = ProcessReportData(std::move(aPayload));
 
 exit:
     ChipLogFunctError(err);
 
-    ClearExistingExchangeContext();
+    // Close the exchange cleanly so that the ExchangeManager will send an ack for the message we just received.
+    mpExchangeCtx->Close();
+    mpExchangeCtx = nullptr;
     MoveToState(ClientState::Initialized);
+
     if (mpDelegate != nullptr)
     {
         if (err != CHIP_NO_ERROR)
@@ -207,7 +220,7 @@ void ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContex
     return;
 }
 
-CHIP_ERROR ReadClient::ClearExistingExchangeContext()
+CHIP_ERROR ReadClient::AbortExistingExchangeContext()
 {
     if (mpExchangeCtx != nullptr)
     {
@@ -302,7 +315,7 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex
 {
     ChipLogProgress(DataManagement, "Time out! failed to receive report data from Exchange: %d",
                     apExchangeContext->GetExchangeId());
-    ClearExistingExchangeContext();
+    AbortExistingExchangeContext();
     MoveToState(ClientState::Initialized);
     if (nullptr != mpDelegate)
     {
diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h
@@ -115,7 +115,7 @@ class ReadClient : public Messaging::ExchangeDelegate
 
     void MoveToState(const ClientState aTargetState);
     CHIP_ERROR ProcessReportData(System::PacketBufferHandle aPayload);
-    CHIP_ERROR ClearExistingExchangeContext();
+    CHIP_ERROR AbortExistingExchangeContext();
     const char * GetStateStr() const;
 
     Messaging::ExchangeManager * mpExchangeMgr = nullptr;
diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -52,15 +52,15 @@ void ReadHandler::Shutdown()
 {
     InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpAttributeClusterInfoList);
     InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpEventClusterInfoList);
-    ClearExistingExchangeContext();
+    AbortExistingExchangeContext();
     MoveToState(HandlerState::Uninitialized);
     mpDelegate                 = nullptr;
     mpAttributeClusterInfoList = nullptr;
     mpEventClusterInfoList     = nullptr;
     mCurrentPriority           = PriorityLevel::Invalid;
 }
 
-CHIP_ERROR ReadHandler::ClearExistingExchangeContext()
+CHIP_ERROR ReadHandler::AbortExistingExchangeContext()
 {
     if (mpExchangeCtx != nullptr)
     {
diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h
@@ -128,7 +128,7 @@ class ReadHandler
     void MoveToState(const HandlerState aTargetState);
 
     const char * GetStateStr() const;
-    CHIP_ERROR ClearExistingExchangeContext();
+    CHIP_ERROR AbortExistingExchangeContext();
 
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;
     InteractionModelDelegate * mpDelegate      = nullptr;

Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ OT_SIMULATION_CACHE="$CIRQUE_CACHE_PATH/ot-simulation.tgz"`
`36`	`36`	`CIRQUE_TESTS=(`
`37`	`37`	`"EchoTest"`
`38`	`38`	`"MobileDeviceTest"`
`39`		`- "InteractionModel"`
	`39`	`+ "InteractionModelTest"`
`40`	`40`	`)`
`41`	`41`
`42`	`42`	`BOLD_GREEN_TEXT="\033[1;32m"`
Original file line number	Diff line number	Diff line change
`@@ -52,15 +52,15 @@ void ReadHandler::Shutdown()`
`52`	`52`	`{`
`53`	`53`	`InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpAttributeClusterInfoList);`
`54`	`54`	`InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpEventClusterInfoList);`
`55`		`- ClearExistingExchangeContext();`
	`55`	`+ AbortExistingExchangeContext();`
`56`	`56`	`MoveToState(HandlerState::Uninitialized);`
`57`	`57`	`mpDelegate = nullptr;`
`58`	`58`	`mpAttributeClusterInfoList = nullptr;`
`59`	`59`	`mpEventClusterInfoList = nullptr;`
`60`	`60`	`mCurrentPriority = PriorityLevel::Invalid;`
`61`	`61`	`}`
`62`	`62`
`63`		`-CHIP_ERROR ReadHandler::ClearExistingExchangeContext()`
	`63`	`+CHIP_ERROR ReadHandler::AbortExistingExchangeContext()`
`64`	`64`	`{`
`65`	`65`	`if (mpExchangeCtx != nullptr)`
`66`	`66`	`{`