address comments

src/app/CommandHandler.cpp

@@ -66,6 +66,9 @@ CHIP_ERROR CommandHandler::AllocateBuffer()
     return CHIP_NO_ERROR;
 }
 
+// The IM engine sends a status response back if it couldn't find a CommandHandler to handle it. But for when it does find one,
+// the responsibility should shift to the handler to handle that, since it may need to do special things to the EC.
+// (like manually calling Close() on it).
 Status CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & payloadHeader,
                                               System::PacketBufferHandle && payload, bool isTimedInvoke)
 {
@@ -107,8 +110,14 @@ Status CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * apExc
         // Null out the (now-closed) exchange, so that when we try to
         // SendCommandResponse() later (when our holdoff count drops to 0) it
         // just fails and we don't double-respond.
+        // The reason that is not calling Close() on the handler object lies at which there is an active CommandHandler::Handle
+        // somewhere in the system for us to even arrive here, since in all normal command processing flows,
+        // the response is dispatched synchronously, and the EC closed synhcronously as well.
         mpExchangeCtx = nullptr;
-        status        = Status::Success;
+
+        // We have sent out the status response, if sending is failing, we don't wanna the caller resending the status respose
+        // again.
+        status = Status::Success;
     }
     return status;
 }

src/app/CommandHandler.h

@@ -239,7 +239,7 @@ class CommandHandler : public Messaging::ExchangeDelegate
     void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override
     {
         VerifyOrDieWithMsg(false, InteractionModel,
-                           "not expect a response, either caused by faulty logic, or at the EC that needs to be fixed.");
+                           "Not expecting a response. This is either caused by faulty IM or EC management logic.");
     }
 
     enum class State

src/app/CommandSender.cpp

@@ -118,7 +118,7 @@ CHIP_ERROR CommandSender::SendInvokeRequest()
 CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
                                             System::PacketBufferHandle && aPayload)
 {
-    bool suppressErrorStatusResponse = false;
+    bool sendStatusResponse = true;
     if (mState == State::CommandSent)
     {
         MoveToState(State::ResponseReceived);
@@ -133,7 +133,7 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
         {
             CHIP_ERROR statusError = CHIP_NO_ERROR;
             SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
-            suppressErrorStatusResponse = true;
+            sendStatusResponse = false;
             SuccessOrExit(err = statusError);
             err = SendInvokeRequest();
         }
@@ -153,6 +153,7 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
     }
     else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
     {
+        // we would not expect to reveive status response in normal flow.
         CHIP_ERROR statusError = CHIP_NO_ERROR;
         SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
         SuccessOrExit(err = statusError);
@@ -164,14 +165,14 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
     }
 
 exit:
-    ResponseMessageHandled(err, apExchangeContext, suppressErrorStatusResponse);
+    ResponseMessageHandled(err, apExchangeContext, sendStatusResponse);
     return err;
 }
 
 void CommandSender::ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
-                                           bool aSuppressErrorStatusResponse)
+                                           bool aSendStatusResponse)
 {
-    if (aError != CHIP_NO_ERROR && !aSuppressErrorStatusResponse)
+    if (aError != CHIP_NO_ERROR && aSendStatusResponse)
     {
         CHIP_ERROR err =
             StatusResponse::Send(Protocols::InteractionModel::Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);

src/app/CommandSender.h

@@ -281,8 +281,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
     CHIP_ERROR SendInvokeRequest();
 
     CHIP_ERROR Finalize(System::PacketBufferHandle & commandPacket);
-    void ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
-                                bool aSuppressErrorStatusResponse);
+    void ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext, bool aSendStatusResponse);
 
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;
     Callback * mpCallback                      = nullptr;

src/app/ReadClient.cpp

@@ -416,6 +416,7 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
     }
     else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
     {
+        // we would not expect to reveive status response in normal flow.
         CHIP_ERROR statusError = CHIP_NO_ERROR;
         SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
         SuccessOrExit(err = statusError);
@@ -597,6 +598,8 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
         }
     }
 
+    // The caller is handling sending StatusResponse in the negative case,
+    // and transmission of StatusResponse in the positive cases are handled here.
     if (!suppressResponse && err == CHIP_NO_ERROR)
     {
         bool noResponseExpected = IsSubscriptionActive() && !mPendingMoreChunks;

src/app/ReadHandler.cpp

@@ -151,11 +151,11 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)
 
 CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
 {
-    CHIP_ERROR err                   = CHIP_NO_ERROR;
-    bool suppressErrorStatusResponse = false;
-    CHIP_ERROR statusError           = CHIP_NO_ERROR;
+    CHIP_ERROR err          = CHIP_NO_ERROR;
+    bool sendStatusResponse = true;
+    CHIP_ERROR statusError  = CHIP_NO_ERROR;
     SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
-    suppressErrorStatusResponse = true;
+    sendStatusResponse = false;
     SuccessOrExit(err = statusError);
     switch (mState)
     {
@@ -208,7 +208,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
 exit:
     if (err != CHIP_NO_ERROR)
     {
-        if (!suppressErrorStatusResponse)
+        if (sendStatusResponse)
         {
             err = StatusResponse::Send(Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);
             if (err == CHIP_NO_ERROR)

src/app/WriteClient.cpp

@@ -448,7 +448,7 @@ CHIP_ERROR WriteClient::SendWriteRequest()
 CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
                                           System::PacketBufferHandle && aPayload)
 {
-    bool suppressErrorStatusResponse = false;
+    bool sendStatusResponse = true;
     if (mState == State::AwaitingResponse &&
         // We had sent the last chunk of data, and received all responses
         mChunks.IsNull())
@@ -469,7 +469,7 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
         {
             CHIP_ERROR statusError = CHIP_NO_ERROR;
             SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
-            suppressErrorStatusResponse = true;
+            sendStatusResponse = false;
             SuccessOrExit(err = statusError);
             err = SendWriteRequest();
         }
@@ -494,6 +494,7 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
     }
     else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
     {
+        // we would not expect to reveive status response in normal flow.
         CHIP_ERROR statusError = CHIP_NO_ERROR;
         SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
         SuccessOrExit(err = statusError);
@@ -505,28 +506,25 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
     }
 
 exit:
-    ResponseMessageHandled(err, apExchangeContext, suppressErrorStatusResponse);
+    ResponseMessageHandled(err, apExchangeContext, sendStatusResponse);
     return err;
 }
 
 void WriteClient::ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
-                                         bool aSuppressErrorStatusResponse)
+                                         bool aSendStatusResponse)
 {
-    if (aError != CHIP_NO_ERROR)
+    if (aError != CHIP_NO_ERROR && aSendStatusResponse)
     {
         CHIP_ERROR err =
             StatusResponse::Send(Protocols::InteractionModel::Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);
-        if (!aSuppressErrorStatusResponse)
+        if (err == CHIP_NO_ERROR)
         {
-            if (err == CHIP_NO_ERROR)
-            {
-                mpExchangeCtx = nullptr;
-            }
+            mpExchangeCtx = nullptr;
+        }
 
-            if (mpCallback != nullptr)
-            {
-                mpCallback->OnError(this, aError);
-            }
+        if (mpCallback != nullptr)
+        {
+            mpCallback->OnError(this, aError);
         }
     }
 

src/app/WriteClient.h

@@ -358,8 +358,7 @@ class WriteClient : public Messaging::ExchangeDelegate
     CHIP_ERROR FinishAttributeIB();
     TLV::TLVWriter * GetAttributeDataIBTLVWriter();
 
-    void ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
-                                bool aSuppressErrorStatusResponse);
+    void ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext, bool aSendStatusResponse);
     /**
      * Create a new message (or a new chunk) for the write request.
      */