diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 22941880551275..3d8d3babd5bea5 100644 --- a/src/app/CommandHandler.cpp +++ b/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; } diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index a0e523b6079b94..45d34e15e455db 100644 --- a/src/app/CommandHandler.h +++ b/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 diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 3bd88aca6ac444..70380e0f8d5bbb 100644 --- a/src/app/CommandSender.cpp +++ b/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*/); diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index 2c7291cec64466..66544d9b737137 100644 --- a/src/app/CommandSender.h +++ b/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; diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 561878351a2f37..081fbc8e229905 100644 --- a/src/app/ReadClient.cpp +++ b/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; diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 343d7208bd6f1c..9e14bb8f190276 100644 --- a/src/app/ReadHandler.cpp +++ b/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) diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index ac6b84211ae0d6..d3b773f54cb306 100644 --- a/src/app/WriteClient.cpp +++ b/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); } } diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h index 1b915de275e59f..ef87a5b5d25ee3 100644 --- a/src/app/WriteClient.h +++ b/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. */