diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 7e6206bc6374ff..b774dcccc65cd1 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -94,6 +94,7 @@ jobs: --known-failure app/AttributeAccessToken.h \ --known-failure app/CommandHandler.h \ --known-failure app/CommandHandlerInterface.h \ + --known-failure app/CommandResponseSender.h \ --known-failure app/CommandSenderLegacyCallback.h \ --known-failure app/data-model/ListLargeSystemExtensions.h \ --known-failure app/ReadHandler.h \ diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 2ac041439aaa5f..6f5b5d60081c72 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -271,9 +271,9 @@ static_library("app") { "ChunkedWriteCallback.cpp", "ChunkedWriteCallback.h", "CommandHandler.cpp", + "CommandHandlerExchangeInterface.h", "CommandResponseHelper.h", "CommandResponseSender.cpp", - "CommandResponseSender.h", "DefaultAttributePersistenceProvider.cpp", "DefaultAttributePersistenceProvider.h", "DeferredAttributePersistenceProvider.cpp", @@ -294,6 +294,7 @@ static_library("app") { # TODO: the following items cannot be included due to interaction-model circularity # (app depending on im and im including these headers): # Name with _ so that linter does not recognize it + # "CommandResponseSender._h" # "CommandHandler._h" # "ReadHandler._h", # "WriteHandler._h" diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 8170b57abd7d23..3d9589bbacb7ec 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -43,19 +43,26 @@ namespace chip { namespace app { using Status = Protocols::InteractionModel::Status; -CommandHandler::CommandHandler(Callback * apCallback) : - mpCallback(apCallback), mResponseSenderDone(HandleOnResponseSenderDone, this), mSuppressResponse(false) -{} +CommandHandler::CommandHandler(Callback * apCallback) : mpCallback(apCallback), mSuppressResponse(false) {} -CommandHandler::CommandHandler(TestOnlyMarker aTestMarker, Callback * apCallback, CommandPathRegistry * apCommandPathRegistry) : - CommandHandler(apCallback) +CommandHandler::CommandHandler(TestOnlyOverrides & aTestOverride, Callback * apCallback) : CommandHandler(apCallback) { - mMaxPathsPerInvoke = apCommandPathRegistry->MaxSize(); - mCommandPathRegistry = apCommandPathRegistry; + if (aTestOverride.commandPathRegistry) + { + mMaxPathsPerInvoke = aTestOverride.commandPathRegistry->MaxSize(); + mCommandPathRegistry = aTestOverride.commandPathRegistry; + } + if (aTestOverride.commandResponder) + { + SetExchangeInterface(aTestOverride.commandResponder); + } } CHIP_ERROR CommandHandler::AllocateBuffer() { + // We should only allocate a buffer if we will be sending out a response. + VerifyOrReturnError(ResponsesAccepted(), CHIP_ERROR_INCORRECT_STATE); + if (!mBufferAllocated) { mCommandMessageWriter.Reset(); @@ -92,40 +99,20 @@ CHIP_ERROR CommandHandler::AllocateBuffer() return CHIP_NO_ERROR; } -void CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, - System::PacketBufferHandle && payload, bool isTimedInvoke) +Status CommandHandler::OnInvokeCommandRequest(CommandHandlerExchangeInterface & commandResponder, + System::PacketBufferHandle && payload, bool isTimedInvoke) { - System::PacketBufferHandle response; - Status status = Status::Failure; - VerifyOrDieWithMsg(ec != nullptr, DataManagement, "Incoming exchange context should not be null"); VerifyOrDieWithMsg(mState == State::Idle, DataManagement, "state should be Idle"); - // NOTE: we already know this is an InvokeCommand Request message because we explicitly registered with the - // Exchange Manager for unsolicited InvokeCommand Requests. - mResponseSender.SetExchangeContext(ec); + SetExchangeInterface(&commandResponder); - // Use the RAII feature, if this is the only Handle when this function returns, DecrementHoldOff will trigger sending response. - // TODO: This is broken! If something under here returns error, we will try - // to StartSendingCommandResponses(), and then our caller will try to send a status - // response too. Figure out at what point it's our responsibility to - // handler errors vs our caller's. + // Using RAII here: if this is the only handle remaining, DecrementHoldOff will + // call the CommandHandler::OnDone callback when this function returns. Handle workHandle(this); - // TODO(#30453): It should be possible for SetExchangeContext to internally call WillSendMessage. - // Unfortunately, doing so would require us to either: - // * Make TestCommandInteraction a friend of CommandResponseSender to allow it to set the exchange - // context without calling WillSendMessage, or - // * Understand why unit tests fail when WillSendMessage is called during the execution of - // SetExchangeContext. - mResponseSender.WillSendMessage(); - status = ProcessInvokeRequest(std::move(payload), isTimedInvoke); - if (status != Status::Success) - { - mResponseSender.SendStatusResponse(status); - mSentStatusResponse = true; - } - - mGoneAsync = true; + Status status = ProcessInvokeRequest(std::move(payload), isTimedInvoke); + mGoneAsync = true; + return status; } CHIP_ERROR CommandHandler::ValidateInvokeRequestMessageAndBuildRegistry(InvokeRequestMessage::Parser & invokeRequestMessage) @@ -209,7 +196,8 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa #if CHIP_CONFIG_IM_PRETTY_PRINT invokeRequestMessage.PrettyPrint(); #endif - if (mResponseSender.IsForGroup()) + VerifyOrDie(mpResponder); + if (mpResponder->GetGroupId().HasValue()) { SetGroupRequest(true); } @@ -270,6 +258,7 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa void CommandHandler::Close() { mSuppressResponse = false; + mpResponder = nullptr; MoveToState(State::AwaitingDestruction); // We must finish all async work before we can shut down a CommandHandler. The actual CommandHandler MUST finish their work @@ -284,14 +273,6 @@ void CommandHandler::Close() } } -void CommandHandler::HandleOnResponseSenderDone(void * context) -{ - CommandHandler * const _this = static_cast(context); - VerifyOrDie(_this != nullptr); - - _this->Close(); -} - void CommandHandler::IncrementHoldOff() { mPendingWork++; @@ -307,57 +288,22 @@ void CommandHandler::DecrementHoldOff() return; } - if (!mSentStatusResponse) + if (mpResponder == nullptr) { - if (!mResponseSender.HasExchangeContext()) + ChipLogProgress(DataManagement, "Skipping command response: response sender is null"); + } + else if (!IsGroupRequest()) + { + CHIP_ERROR err = FinalizeLastInvokeResponseMessage(); + if (err != CHIP_NO_ERROR) { - ChipLogProgress(DataManagement, "Skipping command response: exchange context is null"); - } - else if (!IsGroupRequest()) - { - CHIP_ERROR err = StartSendingCommandResponses(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(DataManagement, "Failed to send command response: %" CHIP_ERROR_FORMAT, err.Format()); - // TODO(#30453): It should be our responsibility to send a Failure StatusResponse to the requestor - // if there is a SessionHandle, but legacy unit tests explicitly check the behavior where - // CommandHandler does not send any message. Changing this behavior should be done in a standalone - // PR where only that specific change is made. Here is a possible solution that should - // be done that fulfills our responsibility to send a Failure StatusResponse, but this causes unit - // tests to start failing. - // ``` - // if (mResponseSender.HasSessionHandle()) - // { - // mResponseSender.SendStatusResponse(Status::Failure); - // } - // Close(); - // return; - // ``` - } + ChipLogError(DataManagement, "Failed to finalize command response: %" CHIP_ERROR_FORMAT, err.Format()); } } - if (mResponseSender.AwaitingStatusResponse()) - { - // If we are awaiting a status response, we want to call Close() only once the response sender is done. - // Therefore, register to be notified when CommandResponseSender is done. - mResponseSender.RegisterOnResponseSenderDoneCallback(&mResponseSenderDone); - return; - } Close(); } -CHIP_ERROR CommandHandler::StartSendingCommandResponses() -{ - VerifyOrReturnError(mPendingWork == 0, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(mResponseSender.HasExchangeContext(), CHIP_ERROR_INCORRECT_STATE); - - ReturnErrorOnFailure(FinalizeLastInvokeResponseMessage()); - ReturnErrorOnFailure(mResponseSender.StartSendingCommandResponses()); - return CHIP_NO_ERROR; -} - namespace { // We use this when the sender did not actually provide a CommandFields struct, // to avoid downstream consumers having to worry about cases when there is or is @@ -396,8 +342,6 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem } } - VerifyOrExit(mResponseSender.HasSessionHandle(), err = CHIP_ERROR_INCORRECT_STATE); - { Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; @@ -485,7 +429,9 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman err = commandPath.GetGroupCommandPath(&clusterId, &commandId); VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); - groupId = mResponseSender.GetGroupId(); + VerifyOrDie(mpResponder); + // The optionalGroupId must have a value, otherwise we wouldn't have reached this code path. + groupId = mpResponder->GetGroupId().Value(); fabric = GetAccessingFabricIndex(); ChipLogDetail(DataManagement, "Received group command for Group=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI, @@ -576,6 +522,9 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman CHIP_ERROR CommandHandler::TryAddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus) { + // Return early when response should not be sent out. + VerifyOrReturnValue(ResponsesAccepted(), CHIP_NO_ERROR); + ReturnErrorOnFailure(PrepareStatus(aCommandPath)); CommandStatusIB::Builder & commandStatus = mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetStatus(); StatusIB::Builder & statusIBBuilder = commandStatus.CreateErrorStatus(); @@ -593,14 +542,14 @@ CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aComman void CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, const char * context) { - // Return early in case of requests targeted to a group, since they should not add a response. - VerifyOrReturn(!IsGroupRequest()); CHIP_ERROR error = FallibleAddStatus(aCommandPath, aStatus, context); if (error != CHIP_NO_ERROR) { ChipLogError(DataManagement, "Failed to add command status: %" CHIP_ERROR_FORMAT, error.Format()); + // TODO(#30453) we could call mpResponder->ResponseDropped() if err == CHIP_ERROR_NO_MEMORY. This should + // be done as a follow up so that change can be evaluated as a standalone PR. // Do not crash if the status has not been added due to running out of packet buffers or other resources. // It is better to drop a single response than to go offline and lose all sessions and subscriptions. @@ -611,7 +560,6 @@ void CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, const P CHIP_ERROR CommandHandler::FallibleAddStatus(const ConcreteCommandPath & path, const Protocols::InteractionModel::Status status, const char * context) { - if (status != Status::Success) { if (context == nullptr) @@ -670,6 +618,9 @@ CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aResponseC CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandPathRegistryEntry & apCommandPathRegistryEntry, const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) { + // Intentionally omitting the ResponsesAccepted early exit. Direct use of PrepareInvokeResponseCommand + // is discouraged, as it often indicates incorrect usage patterns (see GitHub issue #32486). + // If you're encountering CHIP_ERROR_INCORRECT_STATE, refactoring to use AddResponse is recommended. ReturnErrorOnFailure(AllocateBuffer()); if (!mInternalCallToAddResponseData && mState == State::AddedCommand) @@ -714,6 +665,9 @@ CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandPathRegistr CHIP_ERROR CommandHandler::FinishCommand(bool aStartDataStruct) { + // Intentionally omitting the ResponsesAccepted early exit. Direct use of FinishCommand + // is discouraged, as it often indicates incorrect usage patterns (see GitHub issue #32486). + // If you're encountering CHIP_ERROR_INCORRECT_STATE, refactoring to use AddResponse is recommended. VerifyOrReturnError(mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); CommandDataIB::Builder & commandData = mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetCommand(); if (aStartDataStruct) @@ -791,6 +745,7 @@ CHIP_ERROR CommandHandler::RollbackResponse() { VerifyOrReturnError(mRollbackBackupValid, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mState == State::Preparing || mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); + ChipLogDetail(DataManagement, "Rolling back response"); // TODO(#30453): Rollback of mInvokeResponseBuilder should handle resetting // InvokeResponses. mInvokeResponseBuilder.GetInvokeResponses().ResetError(); @@ -813,7 +768,8 @@ TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter() FabricIndex CommandHandler::GetAccessingFabricIndex() const { VerifyOrDie(!mGoneAsync); - return mResponseSender.GetAccessingFabricIndex(); + VerifyOrDie(mpResponder); + return mpResponder->GetAccessingFabricIndex(); } CommandHandler * CommandHandler::Handle::Get() @@ -860,7 +816,8 @@ CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessageAndPrepareNext() // definitively guaranteed. // Response dropping is not yet definitive as a subsequent call // to AllocateBuffer might succeed. - mResponseSender.ResponseDropped(); + VerifyOrDie(mpResponder); + mpResponder->ResponseDropped(); } return err; } @@ -880,12 +837,19 @@ CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessage(bool aHasMoreChunks) } ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage()); ReturnErrorOnFailure(mCommandMessageWriter.Finalize(&packet)); - mResponseSender.AddInvokeResponseToSend(std::move(packet)); + VerifyOrDie(mpResponder); + mpResponder->AddInvokeResponseToSend(std::move(packet)); mBufferAllocated = false; mRollbackBackupValid = false; return CHIP_NO_ERROR; } +void CommandHandler::SetExchangeInterface(CommandHandlerExchangeInterface * commandResponder) +{ + VerifyOrDieWithMsg(mState == State::Idle, DataManagement, "CommandResponseSender can only be set in idle state"); + mpResponder = commandResponder; +} + const char * CommandHandler::GetStateStr() const { #if CHIP_DETAIL_LOGGING @@ -958,20 +922,18 @@ CHIP_ERROR TestOnlyExtractCommandPathFromNextInvokeRequest(TLV::TLVReader & invo // This method intentionally duplicates code from other sections. While code consolidation // is generally preferred, here we prioritize generating a clear crash message to aid in // troubleshooting test failures. -void CommandHandler::TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::ExchangeContext * ec, +void CommandHandler::TestOnlyInvokeCommandRequestWithFaultsInjected(CommandHandlerExchangeInterface & commandResponder, System::PacketBufferHandle && payload, bool isTimedInvoke, NlFaultInjectionType faultType) { - VerifyOrDieWithMsg(ec != nullptr, DataManagement, "TH Failure: Incoming exchange context should not be null"); VerifyOrDieWithMsg(mState == State::Idle, DataManagement, "TH Failure: state should be Idle, issue with TH"); + SetExchangeInterface(&commandResponder); ChipLogProgress(DataManagement, "Response to InvokeRequestMessage overridden by fault injection"); ChipLogProgress(DataManagement, " Injecting the following response:%s", GetFaultInjectionTypeStr(faultType)); - mResponseSender.SetExchangeContext(ec); Handle workHandle(this); - mResponseSender.WillSendMessage(); - VerifyOrDieWithMsg(!mResponseSender.IsForGroup(), DataManagement, "DUT Failure: Unexpected Group Command"); + VerifyOrDieWithMsg(!commandResponder.GetGroupId().HasValue(), DataManagement, "DUT Failure: Unexpected Group Command"); System::PacketBufferTLVReader reader; InvokeRequestMessage::Parser invokeRequestMessage; diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index f7acd709b26808..e31edfd444c629 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -31,8 +31,8 @@ #pragma once #include "CommandPathRegistry.h" -#include "CommandResponseSender.h" +#include #include #include #include @@ -161,7 +161,7 @@ class CommandHandler // add new parameters without there needing to be an ever increasing parameter list with defaults. struct InvokeResponseParameters { - InvokeResponseParameters(ConcreteCommandPath aRequestCommandPath) : mRequestCommandPath(aRequestCommandPath) {} + InvokeResponseParameters(const ConcreteCommandPath & aRequestCommandPath) : mRequestCommandPath(aRequestCommandPath) {} InvokeResponseParameters & SetStartOrEndDataStruct(bool aStartOrEndDataStruct) { @@ -177,8 +177,11 @@ class CommandHandler bool mStartOrEndDataStruct = true; }; - class TestOnlyMarker + struct TestOnlyOverrides { + public: + CommandPathRegistry * commandPathRegistry = nullptr; + CommandHandlerExchangeInterface * commandResponder = nullptr; }; /* @@ -189,32 +192,39 @@ class CommandHandler CommandHandler(Callback * apCallback); /* - * Constructor to override number of supported paths per invoke. + * Constructor to override the number of supported paths per invoke and command responder. + * + * The callback and any pointers passed via TestOnlyOverrides must outlive this + * CommandHandler object. * - * The callback and command path registry passed in has to outlive this CommandHandler object. * For testing purposes. */ - CommandHandler(TestOnlyMarker aTestMarker, Callback * apCallback, CommandPathRegistry * apCommandPathRegistry); + CommandHandler(TestOnlyOverrides & aTestOverride, Callback * apCallback); /* - * Main entrypoint for this class to handle an invoke request. + * Main entrypoint for this class to handle an InvokeRequestMessage. * - * This function will always call the OnDone function above on the registered callback - * before returning. + * This function MAY call the registered OnDone callback before returning. + * To prevent immediate OnDone invocation, callers can wrap their CommandHandler instance + * within a CommandHandler::Handle. * * isTimedInvoke is true if and only if this is part of a Timed Invoke * transaction (i.e. was preceded by a Timed Request). If we reach here, * the timer verification has already been done. + * + * commandResponder handles sending InvokeResponses, added by clusters, to the client. The + * command responder object must outlive this CommandHandler object. It is only safe to + * release after the caller of OnInvokeCommandRequest receives the OnDone callback. */ - void OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, - System::PacketBufferHandle && payload, bool isTimedInvoke); + Protocols::InteractionModel::Status OnInvokeCommandRequest(CommandHandlerExchangeInterface & commandResponder, + System::PacketBufferHandle && payload, bool isTimedInvoke); /** * Checks that all CommandDataIB within InvokeRequests satisfy the spec's general * constraints for CommandDataIB. Additionally checks that InvokeRequestMessage is * properly formatted. * - * This also builds a registry that to ensure that all commands can be responded + * This also builds a registry to ensure that all commands can be responded * to with the data required as per spec. */ CHIP_ERROR ValidateInvokeRequestMessageAndBuildRegistry(InvokeRequestMessage::Parser & invokeRequestMessage); @@ -237,8 +247,6 @@ class CommandHandler CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus); - Protocols::InteractionModel::Status ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke); - /** * This adds a new CommandDataIB element into InvokeResponses for the associated * aRequestCommandPath. This adds up until the `CommandFields` element within @@ -299,54 +307,6 @@ class CommandHandler */ FabricIndex GetAccessingFabricIndex() const; - /** - * @brief Best effort to add InvokeResponse to InvokeResponseMessage. - * - * Tries to add response using lambda. Upon failure to add response, attempts - * to rollback the InvokeResponseMessage to a known good state. If failure is due - * to insufficient space in the current InvokeResponseMessage: - * - Finalizes the current InvokeResponseMessage. - * - Allocates a new InvokeResponseMessage. - * - Reattempts to add the InvokeResponse to the new InvokeResponseMessage. - * - * @param [in] addResponseFunction A lambda function responsible for adding the - * response to the current InvokeResponseMessage. - */ - template - CHIP_ERROR TryAddingResponse(Function && addResponseFunction) - { - // Invalidate any existing rollback backups. The addResponseFunction is - // expected to create a new backup during either PrepareInvokeResponseCommand - // or PrepareStatus execution. Direct invocation of - // CreateBackupForResponseRollback is avoided since the buffer used by - // InvokeResponseMessage might not be allocated until a Prepare* function - // is called. - mRollbackBackupValid = false; - CHIP_ERROR err = addResponseFunction(); - if (err == CHIP_NO_ERROR) - { - return CHIP_NO_ERROR; - } - ReturnErrorOnFailure(RollbackResponse()); - // If we failed to add a command due to lack of space in the - // packet, we will make another attempt to add the response using - // an additional InvokeResponseMessage. - if (mState != State::AddedCommand || err != CHIP_ERROR_NO_MEMORY) - { - return err; - } - ReturnErrorOnFailure(FinalizeInvokeResponseMessageAndPrepareNext()); - err = addResponseFunction(); - if (err != CHIP_NO_ERROR) - { - // The return value of RollbackResponse is ignored, as we prioritize - // conveying the error generated by addResponseFunction to the - // caller. - RollbackResponse(); - } - return err; - } - /** * API for adding a data response. The template parameter T is generally * expected to be a ClusterName::Commands::CommandName::Type struct, but any @@ -360,6 +320,9 @@ class CommandHandler template CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) { + // Return early when response should not be sent out. + VerifyOrReturnValue(ResponsesAccepted(), CHIP_NO_ERROR); + return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, aData); }); } @@ -395,14 +358,18 @@ class CommandHandler * Gets the inner exchange context object, without ownership. * * WARNING: This is dangerous, since it is directly interacting with the - * exchange being managed automatically by mResponseSender and + * exchange being managed automatically by mpResponder and * if not done carefully, may end up with use-after-free errors. * * @return The inner exchange context, might be nullptr if no * exchange context has been assigned or the context * has been released. */ - Messaging::ExchangeContext * GetExchangeContext() const { return mResponseSender.GetExchangeContext(); } + Messaging::ExchangeContext * GetExchangeContext() const + { + VerifyOrDie(mpResponder); + return mpResponder->GetExchangeContext(); + } /** * @brief Flush acks right away for a slow command @@ -415,7 +382,13 @@ class CommandHandler * execution. * */ - void FlushAcksRightAwayOnSlowCommand() { mResponseSender.FlushAcksRightNow(); } + void FlushAcksRightAwayOnSlowCommand() + { + if (mpResponder) + { + mpResponder->HandlingSlowCommand(); + } + } /** * GetSubjectDescriptor() may only be called during synchronous command @@ -426,7 +399,8 @@ class CommandHandler Access::SubjectDescriptor GetSubjectDescriptor() const { VerifyOrDie(!mGoneAsync); - return mResponseSender.GetSubjectDescriptor(); + VerifyOrDie(mpResponder); + return mpResponder->GetSubjectDescriptor(); } #if CHIP_WITH_NLFAULTINJECTION @@ -447,15 +421,16 @@ class CommandHandler * This function strictly validates the DUT's InvokeRequestMessage against the test plan. * If deviations occur, the TH terminates with a detailed error message. * - * @param ec Exchange context for sending InvokeResponseMessages to the client. + * @param commandResponder commandResponder that will send the InvokeResponseMessages to the client. * @param payload Payload of the incoming InvokeRequestMessage from the client. * @param isTimedInvoke Indicates whether the interaction is timed. * @param faultType The specific type of fault to inject into the response. */ // TODO(#30453): After refactoring CommandHandler for better unit testability, create a // unit test specifically for the fault injection behavior. - void TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::ExchangeContext * ec, System::PacketBufferHandle && payload, - bool isTimedInvoke, NlFaultInjectionType faultType); + void TestOnlyInvokeCommandRequestWithFaultsInjected(CommandHandlerExchangeInterface & commandResponder, + System::PacketBufferHandle && payload, bool isTimedInvoke, + NlFaultInjectionType faultType); #endif // CHIP_WITH_NLFAULTINJECTION private: @@ -473,6 +448,54 @@ class CommandHandler AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application. }; + /** + * @brief Best effort to add InvokeResponse to InvokeResponseMessage. + * + * Tries to add response using lambda. Upon failure to add response, attempts + * to rollback the InvokeResponseMessage to a known good state. If failure is due + * to insufficient space in the current InvokeResponseMessage: + * - Finalizes the current InvokeResponseMessage. + * - Allocates a new InvokeResponseMessage. + * - Reattempts to add the InvokeResponse to the new InvokeResponseMessage. + * + * @param [in] addResponseFunction A lambda function responsible for adding the + * response to the current InvokeResponseMessage. + */ + template + CHIP_ERROR TryAddingResponse(Function && addResponseFunction) + { + // Invalidate any existing rollback backups. The addResponseFunction is + // expected to create a new backup during either PrepareInvokeResponseCommand + // or PrepareStatus execution. Direct invocation of + // CreateBackupForResponseRollback is avoided since the buffer used by + // InvokeResponseMessage might not be allocated until a Prepare* function + // is called. + mRollbackBackupValid = false; + CHIP_ERROR err = addResponseFunction(); + if (err == CHIP_NO_ERROR) + { + return CHIP_NO_ERROR; + } + ReturnErrorOnFailure(RollbackResponse()); + // If we failed to add a command due to lack of space in the + // packet, we will make another attempt to add the response using + // an additional InvokeResponseMessage. + if (mState != State::AddedCommand || err != CHIP_ERROR_NO_MEMORY) + { + return err; + } + ReturnErrorOnFailure(FinalizeInvokeResponseMessageAndPrepareNext()); + err = addResponseFunction(); + if (err != CHIP_NO_ERROR) + { + // The return value of RollbackResponse is ignored, as we prioritize + // conveying the error generated by addResponseFunction to the + // caller. + RollbackResponse(); + } + return err; + } + void MoveToState(const State aTargetState); const char * GetStateStr() const; @@ -549,6 +572,8 @@ class CommandHandler CHIP_ERROR FinalizeInvokeResponseMessage(bool aHasMoreChunks); + Protocols::InteractionModel::Status ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke); + /** * Called internally to signal the completion of all work on this object, gracefully close the * exchange (by calling into the base class) and finally, signal to a registerd callback that it's @@ -556,11 +581,6 @@ class CommandHandler */ void Close(); - /** - * @brief Callback method invoked when CommandResponseSender has finished sending all messages. - */ - static void HandleOnResponseSenderDone(void * context); - /** * ProcessCommandDataIB is only called when a unicast invoke command request is received * It requires the endpointId in its command path to be able to dispatch the command @@ -572,7 +592,6 @@ class CommandHandler * It doesn't need the endpointId in it's command path since it uses the GroupId in message metadata to find it */ Protocols::InteractionModel::Status ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement); - CHIP_ERROR StartSendingCommandResponses(); CHIP_ERROR TryAddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus); @@ -595,9 +614,6 @@ class CommandHandler CHIP_ERROR TryAddResponseDataPreEncode(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aResponseCommandPath) { - // Return early in case of requests targeted to a group, since they should not add a response. - VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); - InvokeResponseParameters prepareParams(aRequestCommandPath); prepareParams.SetStartOrEndDataStruct(false); @@ -638,11 +654,15 @@ class CommandHandler return FinishCommand(/* aEndDataStruct = */ false); } + void SetExchangeInterface(CommandHandlerExchangeInterface * commandResponder); + /** * Check whether the InvokeRequest we are handling is targeted to a group. */ bool IsGroupRequest() { return mGroupRequest; } + bool ResponsesAccepted() { return !(mGroupRequest || mpResponder == nullptr); } + /** * Sets the state flag to keep the information that request we are handling is targeted to a group. */ @@ -652,6 +672,8 @@ class CommandHandler size_t MaxPathsPerInvoke() const { return mMaxPathsPerInvoke; } + bool TestOnlyIsInIdleState() const { return mState == State::Idle; } + Callback * mpCallback = nullptr; InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; @@ -666,19 +688,17 @@ class CommandHandler CommandPathRegistry * mCommandPathRegistry = &mBasicCommandPathRegistry; Optional mRefForResponse; - chip::Callback::Callback mResponseSenderDone; - CommandResponseSender mResponseSender; + CommandHandlerExchangeInterface * mpResponder = nullptr; State mState = State::Idle; State mBackupState; ScopedChangeOnly mInternalCallToAddResponseData{ false }; bool mSuppressResponse = false; bool mTimedRequest = false; - bool mSentStatusResponse = false; bool mGroupRequest = false; bool mBufferAllocated = false; bool mReserveSpaceForMoreChunkMessages = false; - // TODO(#30453): We should introduce breaking change where calls to add CommandData + // TODO(#32486): We should introduce breaking change where calls to add CommandData // need to use AddResponse, and not CommandHandler primitives directly using // GetCommandDataIBTLVWriter. bool mRollbackBackupValid = false; diff --git a/src/app/CommandHandlerExchangeInterface.h b/src/app/CommandHandlerExchangeInterface.h new file mode 100644 index 00000000000000..f39a469f35a440 --- /dev/null +++ b/src/app/CommandHandlerExchangeInterface.h @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * All rights reserved. + * + * 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. + */ + +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +namespace chip { +namespace app { + +/** + * Interface for sending InvokeResponseMessage(s). + * + * Provides information about the associated exchange context. + * + * Design Rationale: This interface enhances unit testability and allows applications to + * customize InvokeResponse behavior. For example, a bridge application might locally execute + * a command using cluster APIs without intending to sending a response on an exchange. + * These cluster APIs require providing an instance of CommandHandler where a status response + * is added (see https://github.com/project-chip/connectedhomeip/issues/32030). + */ +class CommandHandlerExchangeInterface +{ +public: + virtual ~CommandHandlerExchangeInterface() = default; + + /** + * Get a non-owning pointer to the exchange context the InvokeRequestMessage was + * delivered on. + * + * @return The exchange context. Might be nullptr if no exchange context has been + * assigned or the context has been released. For example, the exchange + * context might not be assigned in unit tests, or if an application wishes + * to locally execute cluster APIs and still receive response data without + * sending it on an exchange. + */ + virtual Messaging::ExchangeContext * GetExchangeContext() const = 0; + + // TODO(#30453): Follow up refactor. It should be possible to remove + // GetSubjectDescriptor and GetAccessingFabricIndex, as CommandHandler can get these + // values from ExchangeContext. + + /** + * Gets subject descriptor of the exchange. + * + * WARNING: This method should only be called when the caller is certain the + * session has not been evicted. + */ + virtual Access::SubjectDescriptor GetSubjectDescriptor() const = 0; + + /** + * Gets accessing fabic index of the exchange. + * + * WARNING: This method should only be called when the caller is certain the + * session has not been evicted. + */ + virtual FabricIndex GetAccessingFabricIndex() const = 0; + /** + * If session for the exchange is a group session, returns its group ID. Otherwise, + * returns a null optional. + */ + virtual Optional GetGroupId() const = 0; + + /** + * @brief Called to indicate a slow command is being processed. + * + * Enables the exchange to send whatever transport-level acks might be needed without waiting + * for command processing to complete. + */ + virtual void HandlingSlowCommand() = 0; + + /** + * @brief Adds a completed InvokeResponseMessage to the queue for sending to requester. + * + * Called by CommandHandler. + */ + virtual void AddInvokeResponseToSend(System::PacketBufferHandle && aPacket) = 0; + + /** + * @brief Called to indicate that an InvokeResponse was dropped. + * + * Called by CommandHandler to relay this information to the requester. + */ + virtual void ResponseDropped() = 0; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index 5b322c72ce9d06..d7f40caa476c8e 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -38,12 +38,10 @@ CHIP_ERROR CommandResponseSender::OnMessageReceived(Messaging::ExchangeContext * err = statusError; VerifyOrExit(err == CHIP_NO_ERROR, failureStatusToSend.SetValue(Status::InvalidAction)); - // If SendCommandResponse() fails, we are responsible for closing the exchange, - // as stipulated by the API contract. We fulfill this obligation by not sending - // a message expecting a response on the exchange. Since we are in the middle - // of processing an incoming message, the exchange will close itself once we are - // done processing it, if there is no response to wait for at that point. err = SendCommandResponse(); + // If SendCommandResponse() fails, we must close the exchange. We signal the failure to the + // requester with a StatusResponse ('Failure'). Since we're in the middle of processing an + // incoming message, we close the exchange by indicating that we don't expect a further response. VerifyOrExit(err == CHIP_NO_ERROR, failureStatusToSend.SetValue(Status::Failure)); bool moreToSend = !mChunks.IsNull(); @@ -81,13 +79,28 @@ void CommandResponseSender::OnResponseTimeout(Messaging::ExchangeContext * apExc Close(); } -CHIP_ERROR CommandResponseSender::StartSendingCommandResponses() +void CommandResponseSender::StartSendingCommandResponses() { - VerifyOrReturnError(mState == State::ReadyForInvokeResponses, CHIP_ERROR_INCORRECT_STATE); - // If SendCommandResponse() fails, we are obligated to close the exchange as per the API - // contract. However, this method's contract also stipulates that in the event of our - // failure, the caller bears the responsibility of closing the exchange. - ReturnErrorOnFailure(SendCommandResponse()); + VerifyOrDie(mState == State::ReadyForInvokeResponses); + CHIP_ERROR err = SendCommandResponse(); + if (err != CHIP_NO_ERROR) + { + ChipLogError(DataManagement, "Failed to send InvokeResponseMessage"); + // TODO(#30453): It should be our responsibility to send a Failure StatusResponse to the requestor + // if there is a SessionHandle, but legacy unit tests explicitly check the behavior where + // we do not send any message. Changing this behavior should be done in a standalone + // PR where only that specific change is made. Here is a possible solution that could + // be used that fulfills our responsibility to send a Failure StatusResponse. This causes unit + // tests to start failing. + // ``` + // if (mExchangeCtx && mExchangeCtx->HasSessionHandle()) + // { + // SendStatusResponse(Status::Failure); + // } + // ``` + Close(); + return; + } if (HasMoreToSend()) { @@ -98,7 +111,31 @@ CHIP_ERROR CommandResponseSender::StartSendingCommandResponses() { Close(); } - return CHIP_NO_ERROR; +} + +void CommandResponseSender::OnDone(CommandHandler & apCommandObj) +{ + if (mState == State::ErrorSentDelayCloseUntilOnDone) + { + // We have already sent a message to the client indicating that we are not expecting + // a response. + Close(); + return; + } + StartSendingCommandResponses(); +} + +void CommandResponseSender::DispatchCommand(CommandHandler & apCommandObj, const ConcreteCommandPath & aCommandPath, + TLV::TLVReader & apPayload) +{ + VerifyOrReturn(mpCommandHandlerCallback); + mpCommandHandlerCallback->DispatchCommand(apCommandObj, aCommandPath, apPayload); +} + +Status CommandResponseSender::CommandExists(const ConcreteCommandPath & aCommandPath) +{ + VerifyOrReturnValue(mpCommandHandlerCallback, Protocols::InteractionModel::Status::UnsupportedCommand); + return mpCommandHandlerCallback->CommandExists(aCommandPath); } CHIP_ERROR CommandResponseSender::SendCommandResponse() @@ -139,6 +176,9 @@ const char * CommandResponseSender::GetStateStr() const case State::AllInvokeResponsesSent: return "AllInvokeResponsesSent"; + + case State::ErrorSentDelayCloseUntilOnDone: + return "ErrorSentDelayCloseUntilOnDone"; } #endif // CHIP_DETAIL_LOGGING return "N/A"; @@ -157,12 +197,52 @@ void CommandResponseSender::MoveToState(const State aTargetState) void CommandResponseSender::Close() { MoveToState(State::AllInvokeResponsesSent); - mCloseCalled = true; - if (mResponseSenderDoneCallback) + mpCallback->OnDone(*this); +} + +void CommandResponseSender::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, System::PacketBufferHandle && payload, + bool isTimedInvoke) +{ + VerifyOrDieWithMsg(ec != nullptr, DataManagement, "Incoming exchange context should not be null"); + VerifyOrDieWithMsg(mState == State::ReadyForInvokeResponses, DataManagement, "state should be ReadyForInvokeResponses"); + + // NOTE: we already know this is an InvokeRequestMessage because we explicitly registered with the + // Exchange Manager for unsolicited InvokeRequestMessages. + mExchangeCtx.Grab(ec); + mExchangeCtx->WillSendMessage(); + + // Grabbing Handle to prevent mCommandHandler from calling OnDone before OnInvokeCommandRequest returns. + // This allows us to send a StatusResponse error instead of any potentially queued up InvokeResponseMessages. + CommandHandler::Handle workHandle(&mCommandHandler); + Status status = mCommandHandler.OnInvokeCommandRequest(*this, std::move(payload), isTimedInvoke); + if (status != Status::Success) { - mResponseSenderDoneCallback->mCall(mResponseSenderDoneCallback->mContext); + VerifyOrDie(mState == State::ReadyForInvokeResponses); + SendStatusResponse(status); + // The API contract of OnInvokeCommandRequest requires the CommandResponder instance to outlive + // the CommandHandler. Therefore, we cannot safely call Close() here, even though we have + // finished sending data. Closing must be deferred until the CommandHandler::OnDone callback. + MoveToState(State::ErrorSentDelayCloseUntilOnDone); } } +#if CHIP_WITH_NLFAULTINJECTION + +void CommandResponseSender::TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::ExchangeContext * ec, + System::PacketBufferHandle && payload, + bool isTimedInvoke, + CommandHandler::NlFaultInjectionType faultType) +{ + VerifyOrDieWithMsg(ec != nullptr, DataManagement, "TH Failure: Incoming exchange context should not be null"); + VerifyOrDieWithMsg(mState == State::ReadyForInvokeResponses, DataManagement, + "TH Failure: state should be ReadyForInvokeResponses, issue with TH"); + + mExchangeCtx.Grab(ec); + mExchangeCtx->WillSendMessage(); + + mCommandHandler.TestOnlyInvokeCommandRequestWithFaultsInjected(*this, std::move(payload), isTimedInvoke, faultType); +} +#endif // CHIP_WITH_NLFAULTINJECTION + } // namespace app } // namespace chip diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index 0954a3e2ee0687..a89970ddec6c92 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -17,6 +17,8 @@ #pragma once +#include +#include #include #include #include @@ -24,22 +26,46 @@ namespace chip { namespace app { -typedef void (*OnResponseSenderDone)(void * context); -class CommandHandler; - +// TODO(#30453): Rename CommandResponseSender to CommandResponder in follow up PR /** - * Class manages the process of sending `InvokeResponseMessage`(s) back to the initial requester. + * Manages the process of sending InvokeResponseMessage(s) to the requester. + * + * Implements the CommandHandlerExchangeInterface. Uses a CommandHandler member to process + * InvokeCommandRequest. The CommandHandler is provided a reference to this + * CommandHandlerExchangeInterface implementation to enable sending InvokeResponseMessage(s). */ -class CommandResponseSender : public Messaging::ExchangeDelegate +class CommandResponseSender : public Messaging::ExchangeDelegate, + public CommandHandler::Callback, + public CommandHandlerExchangeInterface { public: - CommandResponseSender() : mExchangeCtx(*this) {} + class Callback + { + public: + virtual ~Callback() = default; + /* + * Signals registered callback that this object has finished its work and can now be + * safely destroyed/released. + */ + virtual void OnDone(CommandResponseSender & apResponderObj) = 0; + }; + + CommandResponseSender(Callback * apCallback, CommandHandler::Callback * apDispatchCallback) : + mpCallback(apCallback), mpCommandHandlerCallback(apDispatchCallback), mCommandHandler(this), mExchangeCtx(*this) + {} CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) override; void OnResponseTimeout(Messaging::ExchangeContext * ec) override; + void OnDone(CommandHandler & apCommandObj) override; + + void DispatchCommand(CommandHandler & apCommandObj, const ConcreteCommandPath & aCommandPath, + TLV::TLVReader & apPayload) override; + + Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; + /** * Gets the inner exchange context object, without ownership. * @@ -51,18 +77,7 @@ class CommandResponseSender : public Messaging::ExchangeDelegate * exchange context has been assigned or the context * has been released. */ - Messaging::ExchangeContext * GetExchangeContext() const { return mExchangeCtx.Get(); } - - /** - * @brief Flush acks right now, typically done when processing a slow command - */ - void FlushAcksRightNow() - { - VerifyOrReturn(mExchangeCtx); - auto * msgContext = mExchangeCtx->GetReliableMessageContext(); - VerifyOrReturn(msgContext != nullptr); - msgContext->FlushAcks(); - } + Messaging::ExchangeContext * GetExchangeContext() const override { return mExchangeCtx.Get(); } /** * Gets subject descriptor of the exchange. @@ -70,84 +85,95 @@ class CommandResponseSender : public Messaging::ExchangeDelegate * WARNING: This method should only be called when the caller is certain the * session has not been evicted. */ - Access::SubjectDescriptor GetSubjectDescriptor() const + Access::SubjectDescriptor GetSubjectDescriptor() const override { VerifyOrDie(mExchangeCtx); return mExchangeCtx->GetSessionHandle()->GetSubjectDescriptor(); } - bool HasSessionHandle() { return mExchangeCtx && mExchangeCtx->HasSessionHandle(); } - - FabricIndex GetAccessingFabricIndex() const + FabricIndex GetAccessingFabricIndex() const override { VerifyOrDie(mExchangeCtx); return mExchangeCtx->GetSessionHandle()->GetFabricIndex(); } - void SetExchangeContext(Messaging::ExchangeContext * ec) { mExchangeCtx.Grab(ec); } - - void WillSendMessage() { mExchangeCtx->WillSendMessage(); } - - bool IsForGroup() + Optional GetGroupId() const override { VerifyOrDie(mExchangeCtx); - return mExchangeCtx->IsGroupExchangeContext(); + auto sessionHandle = mExchangeCtx->GetSessionHandle(); + if (sessionHandle->GetSessionType() != Transport::Session::SessionType::kGroupIncoming) + { + return NullOptional; + } + return MakeOptional(sessionHandle->AsIncomingGroupSession()->GetGroupId()); } - bool HasExchangeContext() { return mExchangeCtx.Get() != nullptr; } - - GroupId GetGroupId() - { - VerifyOrDie(mExchangeCtx); - return mExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId(); - } - - /** - * @brief Initiates the sending of InvokeResponses previously queued using AddInvokeResponseToSend. - * - * Upon failure, the caller is responsible for closing the exchange appropriately, potentially - * by calling `SendStatusResponse`. - */ - CHIP_ERROR StartSendingCommandResponses(); - - void SendStatusResponse(Protocols::InteractionModel::Status aStatus) + void HandlingSlowCommand() override { - StatusResponse::Send(aStatus, mExchangeCtx.Get(), /*aExpectResponse = */ false); + VerifyOrReturn(mExchangeCtx); + auto * msgContext = mExchangeCtx->GetReliableMessageContext(); + VerifyOrReturn(msgContext != nullptr); + msgContext->FlushAcks(); } - bool AwaitingStatusResponse() { return mState == State::AwaitingStatusResponse; } - - void AddInvokeResponseToSend(System::PacketBufferHandle && aPacket) + void AddInvokeResponseToSend(System::PacketBufferHandle && aPacket) override { VerifyOrDie(mState == State::ReadyForInvokeResponses); mChunks.AddToEnd(std::move(aPacket)); } - /** - * @brief Called to indicate that response was dropped + void ResponseDropped() override { mReportResponseDropped = true; } + + /* + * Main entrypoint for this class to handle an invoke request. + * + * isTimedInvoke is true if and only if this is part of a Timed Invoke + * transaction (i.e. was preceded by a Timed Request). If we reach here, + * the timer verification has already been done. */ - void ResponseDropped() { mReportResponseDropped = true; } + void OnInvokeCommandRequest(Messaging::ExchangeContext * ec, System::PacketBufferHandle && payload, bool isTimedInvoke); +#if CHIP_WITH_NLFAULTINJECTION /** - * @brief Registers a callback to be invoked when CommandResponseSender has finished sending responses. + * @brief Sends InvokeResponseMessages with injected faults for certification testing. + * + * The Test Harness (TH) uses this to simulate various server response behaviors, + * ensuring the Device Under Test (DUT) handles responses per specification. + * + * This function strictly validates the DUT's InvokeRequestMessage against the test plan. + * If deviations occur, the TH terminates with a detailed error message. + * + * @param ec Exchange context for sending InvokeResponseMessages to the client. + * @param payload Payload of the incoming InvokeRequestMessage from the client. + * @param isTimedInvoke Indicates whether the interaction is timed. + * @param faultType The specific type of fault to inject into the response. */ - void RegisterOnResponseSenderDoneCallback(Callback::Callback * aResponseSenderDoneCallback) - { - VerifyOrDie(!mCloseCalled); - mResponseSenderDoneCallback = aResponseSenderDoneCallback; - } + void TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::ExchangeContext * ec, System::PacketBufferHandle && payload, + bool isTimedInvoke, CommandHandler::NlFaultInjectionType faultType); +#endif // CHIP_WITH_NLFAULTINJECTION private: enum class State : uint8_t { - ReadyForInvokeResponses, ///< Accepting InvokeResponses to send back to requester. - AwaitingStatusResponse, ///< Awaiting status response from requester, after sending InvokeResponse. - AllInvokeResponsesSent, ///< All InvokeResponses have been sent out. + ReadyForInvokeResponses, ///< Accepting InvokeResponses to send back to requester. + AwaitingStatusResponse, ///< Awaiting status response from requester, after sending InvokeResponse. + AllInvokeResponsesSent, ///< All InvokeResponses have been sent out. + ErrorSentDelayCloseUntilOnDone ///< We have sent an early error response, but still need to clean up. }; void MoveToState(const State aTargetState); const char * GetStateStr() const; + /** + * @brief Initiates the sending of InvokeResponses previously queued using AddInvokeResponseToSend. + */ + void StartSendingCommandResponses(); + + void SendStatusResponse(Protocols::InteractionModel::Status aStatus) + { + StatusResponse::Send(aStatus, mExchangeCtx.Get(), /*aExpectResponse = */ false); + } + CHIP_ERROR SendCommandResponse(); bool HasMoreToSend() { return !mChunks.IsNull() || mReportResponseDropped; } void Close(); @@ -155,11 +181,12 @@ class CommandResponseSender : public Messaging::ExchangeDelegate // A list of InvokeResponseMessages to be sent out by CommandResponseSender. System::PacketBufferHandle mChunks; - chip::Callback::Callback * mResponseSenderDoneCallback = nullptr; + Callback * mpCallback; + CommandHandler::Callback * mpCommandHandlerCallback; + CommandHandler mCommandHandler; Messaging::ExchangeHolder mExchangeCtx; State mState = State::ReadyForInvokeResponses; - bool mCloseCalled = false; bool mReportResponseDropped = false; }; diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 68ab4525d083e4..7a085a772658b8 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -112,7 +112,7 @@ void InteractionModelEngine::Shutdown() // Increase magic number to invalidate all Handle-s. mMagic++; - mCommandHandlerObjs.ReleaseAll(); + mCommandResponderObjs.ReleaseAll(); mTimedHandlers.ForEachActiveObject([this](TimedHandler * obj) -> Loop { mpExchangeMgr->CloseAllContextsForDelegate(obj); @@ -374,9 +374,17 @@ bool InteractionModelEngine::SubjectHasPersistedSubscription(FabricIndex aFabric return false; } +void InteractionModelEngine::OnDone(CommandResponseSender & apResponderObj) +{ + mCommandResponderObjs.ReleaseObject(&apResponderObj); +} + +// TODO(#30453): Follow up refactor. Remove need for InteractionModelEngine::OnDone(CommandHandler). void InteractionModelEngine::OnDone(CommandHandler & apCommandObj) { - mCommandHandlerObjs.ReleaseObject(&apCommandObj); + // We are no longer expecting to receive this callback. With the introduction of CommandResponseSender, it is now + // responsible for receiving this callback. + VerifyOrDie(false); } void InteractionModelEngine::OnDone(ReadHandler & apReadObj) @@ -412,28 +420,29 @@ Status InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload, bool aIsTimedInvoke) { - CommandHandler * commandHandler = mCommandHandlerObjs.CreateObject(this); - if (commandHandler == nullptr) + // TODO(#30453): Refactor CommandResponseSender's constructor to accept an exchange context parameter. + CommandResponseSender * commandResponder = mCommandResponderObjs.CreateObject(this, this); + if (commandResponder == nullptr) { ChipLogProgress(InteractionModel, "no resource for Invoke interaction"); return Status::Busy; } CHIP_FAULT_INJECT( FaultInjection::kFault_IMInvoke_SeparateResponses, - commandHandler->TestOnlyInvokeCommandRequestWithFaultsInjected( + commandResponder->TestOnlyInvokeCommandRequestWithFaultsInjected( apExchangeContext, std::move(aPayload), aIsTimedInvoke, CommandHandler::NlFaultInjectionType::SeparateResponseMessages); return Status::Success;); CHIP_FAULT_INJECT(FaultInjection::kFault_IMInvoke_SeparateResponsesInvertResponseOrder, - commandHandler->TestOnlyInvokeCommandRequestWithFaultsInjected( + commandResponder->TestOnlyInvokeCommandRequestWithFaultsInjected( apExchangeContext, std::move(aPayload), aIsTimedInvoke, CommandHandler::NlFaultInjectionType::SeparateResponseMessagesAndInvertedResponseOrder); return Status::Success;); CHIP_FAULT_INJECT( FaultInjection::kFault_IMInvoke_SkipSecondResponse, - commandHandler->TestOnlyInvokeCommandRequestWithFaultsInjected(apExchangeContext, std::move(aPayload), aIsTimedInvoke, - CommandHandler::NlFaultInjectionType::SkipSecondResponse); + commandResponder->TestOnlyInvokeCommandRequestWithFaultsInjected(apExchangeContext, std::move(aPayload), aIsTimedInvoke, + CommandHandler::NlFaultInjectionType::SkipSecondResponse); return Status::Success;); - commandHandler->OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), aIsTimedInvoke); + commandResponder->OnInvokeCommandRequest(apExchangeContext, std::move(aPayload), aIsTimedInvoke); return Status::Success; } diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 22ec1fb758153a..3f905cb760f174 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -77,6 +78,7 @@ namespace app { */ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, public Messaging::ExchangeDelegate, + public CommandResponseSender::Callback, public CommandHandler::Callback, public ReadHandler::ManagementCallback, public FabricTable::Delegate, @@ -402,6 +404,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, friend class SubscriptionResumptionSessionEstablisher; using Status = Protocols::InteractionModel::Status; + void OnDone(CommandResponseSender & apResponderObj) override; void OnDone(CommandHandler & apCommandObj) override; void OnDone(ReadHandler & apReadObj) override; @@ -599,7 +602,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, CommandHandlerInterface * mCommandHandlerList = nullptr; - ObjectPool mCommandHandlerObjs; + ObjectPool mCommandResponderObjs; ObjectPool mTimedHandlers; WriteHandler mWriteHandlers[CHIP_IM_MAX_NUM_WRITE_HANDLER]; reporting::Engine mReportingEngine; diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 87737cda48d94e..9837818862c21b 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -85,7 +85,6 @@ constexpr CommandId kTestCommandIdCommandSpecificResponse = 6; constexpr CommandId kTestCommandIdFillResponseMessage = 7; constexpr CommandId kTestNonExistCommandId = 0; -const app::CommandHandler::TestOnlyMarker kCommandHandlerTestOnlyMarker; const app::CommandSender::TestOnlyMarker kCommandSenderTestOnlyMarker; } // namespace @@ -284,6 +283,23 @@ class MockCommandSenderExtendableCallback : public CommandSender::ExtendableCall CHIP_ERROR mError = CHIP_NO_ERROR; } mockCommandSenderExtendedDelegate; +class MockCommandResponder : public CommandHandlerExchangeInterface +{ +public: + Messaging::ExchangeContext * GetExchangeContext() const override { return nullptr; } + void HandlingSlowCommand() override {} + Access::SubjectDescriptor GetSubjectDescriptor() const override { return Access::SubjectDescriptor(); } + FabricIndex GetAccessingFabricIndex() const override { return kUndefinedFabricIndex; } + + Optional GetGroupId() const override { return NullOptional; } + + void AddInvokeResponseToSend(System::PacketBufferHandle && aPacket) override { mChunks.AddToEnd(std::move(aPacket)); } + void ResponseDropped() override { mResponseDropped = true; } + + System::PacketBufferHandle mChunks; + bool mResponseDropped = false; +}; + class MockCommandHandlerCallback : public CommandHandler::Callback { public: @@ -313,7 +329,7 @@ class TestCommandInteraction static void TestCommandSenderExtendableApiWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderExtendableApiWithProcessReceivedMsgContainingInvalidCommandRef(nlTestSuite * apSuite, void * apContext); - static void TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerWithOnInvokeReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerEncodeSimpleCommandData(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext); @@ -324,9 +340,11 @@ class TestCommandInteraction static void TestCommandHandlerInvalidMessageSync(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerCommandEncodeExternalFailure(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerEncodeSimpleStatusCode(nlTestSuite * apSuite, void * apContext); - static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerWithoutResponderCallingAddStatus(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerWithoutResponderCallingAddResponse(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerWithoutResponderCallingDirectPrepareFinishCommandApis(nlTestSuite * apSuite, void * apContext); - static void TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerWithOnInvokeReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerRejectMultipleIdenticalCommands(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne(nlTestSuite * apSuite, void * apContext); @@ -354,28 +372,30 @@ class TestCommandInteraction static void TestCommandSenderAbruptDestruction(nlTestSuite * apSuite, void * apContext); - static size_t GetNumActiveHandlerObjects() + static size_t GetNumActiveCommandResponderObjects() { - return chip::app::InteractionModelEngine::GetInstance()->mCommandHandlerObjs.Allocated(); + return chip::app::InteractionModelEngine::GetInstance()->mCommandResponderObjs.Allocated(); } private: /** * With the introduction of batch invoke commands, CommandHandler keeps track of incoming - * ConcreteCommandPath and the associated CommandRef. Normally this is populated as a part - * of OnMessageReceived from the incoming request. For some unit tests were we want to just - * test some of the processing messages we need to inject commands into the table of - * ConcreteCommandPath/CommandRef pairs. + * ConcreteCommandPath and the associated CommandRefs. These are normally populated + * as part of OnInvokeCommandRequest from the incoming request. For some unit tests where + * we want to test APIs that cluster code uses, we need to inject entries into the + * CommandPathRegistry directly. */ - class CommandHandlerWithOutstandingCommand : public app::CommandHandler + class CommandHandlerWithUnrespondedCommand : public app::CommandHandler { public: - CommandHandlerWithOutstandingCommand(CommandHandler::Callback * apCallback, const ConcreteCommandPath & aRequestCommandPath, + CommandHandlerWithUnrespondedCommand(CommandHandler::Callback * apCallback, const ConcreteCommandPath & aRequestCommandPath, const Optional & aRef) : CommandHandler(apCallback) { GetCommandPathRegistry().Add(aRequestCommandPath, aRef); + SetExchangeInterface(&mMockCommandResponder); } + MockCommandResponder mMockCommandResponder; }; // Generate an invoke request. If aCommandId is kTestCommandIdWithData, a @@ -605,15 +625,17 @@ uint32_t TestCommandInteraction::GetAddResponseDataOverheadSizeForPath(nlTestSui const ConcreteCommandPath & aRequestCommandPath, ForcedSizeBufferLengthHint aBufferSizeHint) { - BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + BasicCommandPathRegistry<4> basicCommandPathRegistry; + MockCommandResponder mockCommandResponder; + CommandHandler::TestOnlyOverrides testOnlyOverrides{ &basicCommandPathRegistry, &mockCommandResponder }; + CommandHandler commandHandler(testOnlyOverrides, &mockCommandHandlerDelegate); commandHandler.mReserveSpaceForMoreChunkMessages = true; ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; - CHIP_ERROR err = mBasicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); + CHIP_ERROR err = basicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = mBasicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); + err = basicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); err = commandHandler.AllocateBuffer(); @@ -671,34 +693,23 @@ void TestCommandInteraction::TestCommandSenderWithWrongState(nlTestSuite * apSui void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; - CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, + CommandHandlerWithUnrespondedCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, /* aRef = */ NullOptional); - const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); - err = commandHandler.PrepareInvokeResponseCommand(responseCommandPath, prepareParams); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - - TestExchangeDelegate delegate; - - auto exchange = ctx.NewExchangeToAlice(&delegate, false); - commandHandler.mResponseSender.SetExchangeContext(exchange); - - err = commandHandler.StartSendingCommandResponses(); - - NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); + { + // This simulates how cluster would call CommandHandler APIs synchronously. There would + // be handle already acquired on the callers behalf. + CommandHandler::Handle handle(&commandHandler); - // - // Ordinarily, the ExchangeContext will close itself upon sending the final message / error'ing out on a responder exchange - // when unwinding back from an OnMessageReceived callback. Since that isn't the case in this artificial setup here - // (where we created a responder exchange that's not responding to anything), we need - // to explicitly close it out. This is not expected in normal application logic. - // - exchange->Close(); + const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); + err = commandHandler.PrepareInvokeResponseCommand(responseCommandPath, prepareParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + } + NL_TEST_ASSERT(apSuite, commandHandler.mMockCommandResponder.mChunks.IsNull()); } void TestCommandInteraction::TestCommandSenderWithSendCommand(nlTestSuite * apSuite, void * apContext) @@ -725,29 +736,25 @@ void TestCommandInteraction::TestCommandSenderWithSendCommand(nlTestSuite * apSu void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; - CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, + CommandHandlerWithUnrespondedCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, /* aRef = */ NullOptional); - System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); - TestExchangeDelegate delegate; - auto exchange = ctx.NewExchangeToAlice(&delegate, false); - commandHandler.mResponseSender.SetExchangeContext(exchange); - - const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); - err = commandHandler.PrepareInvokeResponseCommand(responseCommandPath, prepareParams); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = commandHandler.FinishCommand(); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = commandHandler.StartSendingCommandResponses(); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + { + // This simulates how cluster would call CommandHandler APIs synchronously. There would + // be handle already acquired on the callers behalf. + CommandHandler::Handle handle(&commandHandler); - commandHandler.Close(); - ctx.GetLoopback().Reset(); + const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); + err = commandHandler.PrepareInvokeResponseCommand(responseCommandPath, prepareParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + err = commandHandler.FinishCommand(); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + } + NL_TEST_ASSERT(apSuite, !commandHandler.mMockCommandResponder.mChunks.IsNull()); } void TestCommandInteraction::TestCommandSenderWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext) @@ -834,26 +841,18 @@ void TestCommandInteraction::TestCommandSenderExtendableApiWithProcessReceivedMs void TestCommandInteraction::ValidateCommandHandlerEncodeInvokeResponseMessage(nlTestSuite * apSuite, void * apContext, bool aNeedStatusCode) { - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; chip::app::ConcreteCommandPath requestCommandPath(kTestEndpointId, kTestClusterId, kTestCommandIdWithData); - CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, + CommandHandlerWithUnrespondedCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, /* aRef = */ NullOptional); - TestExchangeDelegate delegate; - auto exchange = ctx.NewExchangeToAlice(&delegate, false); - commandHandler.mResponseSender.SetExchangeContext(exchange); - - AddInvokeResponseData(apSuite, apContext, &commandHandler, aNeedStatusCode); - err = commandHandler.FinalizeLastInvokeResponseMessage(); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + { + // This simulates how cluster would call CommandHandler APIs synchronously. There would + // be handle already acquired on the callers behalf. + CommandHandler::Handle handle(&commandHandler); - // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an - // OnMessageReceived callback and not having sent a subsequent message. Since that isn't the case in this artificial setup here - // (where we created a responder exchange that's not responding to anything), we need to explicitly close it out. This is not - // expected in normal application logic. - // - exchange->Close(); + AddInvokeResponseData(apSuite, apContext, &commandHandler, aNeedStatusCode); + } + NL_TEST_ASSERT(apSuite, !commandHandler.mMockCommandResponder.mChunks.IsNull()); } void TestCommandInteraction::TestCommandHandlerEncodeSimpleCommandData(nlTestSuite * apSuite, void * apContext) @@ -893,26 +892,34 @@ struct BadFields void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext) { - CHIP_ERROR err = CHIP_NO_ERROR; auto path = MakeTestCommandPath(); auto requestCommandPath = ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId); - CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, /* aRef = */ NullOptional); + CommandHandlerWithUnrespondedCommand commandHandler(nullptr, requestCommandPath, /* aRef = */ NullOptional); - commandHandler.AddResponse(requestCommandPath, Fields()); - err = commandHandler.FinalizeLastInvokeResponseMessage(); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + { + // This simulates how cluster would call CommandHandler APIs synchronously. There would + // be handle already acquired on the callers behalf. + CommandHandler::Handle handle(&commandHandler); + + commandHandler.AddResponse(requestCommandPath, Fields()); + } + NL_TEST_ASSERT(apSuite, !commandHandler.mMockCommandResponder.mChunks.IsNull()); } void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext) { - CHIP_ERROR err = CHIP_NO_ERROR; auto path = MakeTestCommandPath(); auto requestCommandPath = ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId); - CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, NullOptional); + CommandHandlerWithUnrespondedCommand commandHandler(nullptr, requestCommandPath, NullOptional); - commandHandler.AddResponse(requestCommandPath, BadFields()); - err = commandHandler.FinalizeLastInvokeResponseMessage(); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + { + // This simulates how cluster would call CommandHandler APIs synchronously. There would + // be handle already acquired on the callers behalf. + CommandHandler::Handle handle(&commandHandler); + + commandHandler.AddResponse(requestCommandPath, BadFields()); + } + NL_TEST_ASSERT(apSuite, !commandHandler.mMockCommandResponder.mChunks.IsNull()); } /** @@ -975,7 +982,7 @@ void TestCommandInteraction::TestCommandInvalidMessage1(nlTestSuite * apSuite, v mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 0 && mockCommandSenderDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); NL_TEST_ASSERT(apSuite, !msgBuf.IsNull()); @@ -1015,7 +1022,7 @@ void TestCommandInteraction::TestCommandInvalidMessage1(nlTestSuite * apSuite, v // Client sent status report with invalid action, server's exchange has been closed, so all it sent is an MRP Ack NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); CheckForInvalidAction(apSuite, messageLog); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); ctx.ExpireSessionAliceToBob(); ctx.ExpireSessionBobToAlice(); ctx.CreateSessionAliceToBob(); @@ -1048,7 +1055,7 @@ void TestCommandInteraction::TestCommandInvalidMessage2(nlTestSuite * apSuite, v mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 0 && mockCommandSenderDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); NL_TEST_ASSERT(apSuite, !msgBuf.IsNull()); @@ -1086,7 +1093,7 @@ void TestCommandInteraction::TestCommandInvalidMessage2(nlTestSuite * apSuite, v // Client sent status report with invalid action, server's exchange has been closed, so all it sent is an MRP Ack NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); CheckForInvalidAction(apSuite, messageLog); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); ctx.ExpireSessionAliceToBob(); ctx.ExpireSessionBobToAlice(); ctx.CreateSessionAliceToBob(); @@ -1119,7 +1126,7 @@ void TestCommandInteraction::TestCommandInvalidMessage3(nlTestSuite * apSuite, v mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 0 && mockCommandSenderDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); NL_TEST_ASSERT(apSuite, !msgBuf.IsNull()); @@ -1157,7 +1164,7 @@ void TestCommandInteraction::TestCommandInvalidMessage3(nlTestSuite * apSuite, v // Client sent status report with invalid action, server's exchange has been closed, so all it sent is an MRP Ack NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); CheckForInvalidAction(apSuite, messageLog); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); ctx.ExpireSessionAliceToBob(); ctx.ExpireSessionBobToAlice(); ctx.CreateSessionAliceToBob(); @@ -1190,7 +1197,7 @@ void TestCommandInteraction::TestCommandInvalidMessage4(nlTestSuite * apSuite, v mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 0 && mockCommandSenderDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); NL_TEST_ASSERT(apSuite, !msgBuf.IsNull()); @@ -1228,7 +1235,7 @@ void TestCommandInteraction::TestCommandInvalidMessage4(nlTestSuite * apSuite, v // Client sent status report with invalid action, server's exchange has been closed, so all it sent is an MRP Ack NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); CheckForInvalidAction(apSuite, messageLog); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); ctx.ExpireSessionAliceToBob(); ctx.ExpireSessionBobToAlice(); ctx.CreateSessionAliceToBob(); @@ -1256,7 +1263,7 @@ void TestCommandInteraction::TestCommandHandlerInvalidMessageSync(nlTestSuite * mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 1 && mockCommandSenderDelegate.onErrorCalledTimes == 1); NL_TEST_ASSERT(apSuite, mockCommandSenderDelegate.mError == CHIP_IM_GLOBAL_STATUS(InvalidAction)); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1265,13 +1272,18 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTe CHIP_ERROR err = CHIP_NO_ERROR; auto path = MakeTestCommandPath(); auto requestCommandPath = ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId); - CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, NullOptional); + CommandHandlerWithUnrespondedCommand commandHandler(nullptr, requestCommandPath, NullOptional); - err = commandHandler.AddResponseData(requestCommandPath, BadFields()); - NL_TEST_ASSERT(apSuite, err != CHIP_NO_ERROR); - commandHandler.AddStatus(requestCommandPath, Protocols::InteractionModel::Status::Failure); - err = commandHandler.FinalizeLastInvokeResponseMessage(); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + { + // This simulates how cluster would call CommandHandler APIs synchronously. There would + // be handle already acquired on the callers behalf. + CommandHandler::Handle handle(&commandHandler); + + err = commandHandler.AddResponseData(requestCommandPath, BadFields()); + NL_TEST_ASSERT(apSuite, err != CHIP_NO_ERROR); + commandHandler.AddStatus(requestCommandPath, Protocols::InteractionModel::Status::Failure); + } + NL_TEST_ASSERT(apSuite, !commandHandler.mMockCommandResponder.mChunks.IsNull()); } void TestCommandInteraction::TestCommandHandlerEncodeSimpleStatusCode(nlTestSuite * apSuite, void * apContext) @@ -1280,61 +1292,102 @@ void TestCommandInteraction::TestCommandHandlerEncodeSimpleStatusCode(nlTestSuit ValidateCommandHandlerEncodeInvokeResponseMessage(apSuite, apContext, true /*aNeedStatusCode=true*/); } -void TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext) +void TestCommandInteraction::TestCommandHandlerWithoutResponderCallingAddStatus(nlTestSuite * apSuite, void * apContext) +{ + chip::app::ConcreteCommandPath requestCommandPath(kTestEndpointId, kTestClusterId, kTestCommandIdWithData); + CommandHandler commandHandler(&mockCommandHandlerDelegate); + + commandHandler.AddStatus(requestCommandPath, Protocols::InteractionModel::Status::Failure); + + // Since calling AddStatus is supposed to be a no-operation when there is no responder, it is + // hard to validate. Best way is to check that we are still in an Idle state afterwards + NL_TEST_ASSERT(apSuite, commandHandler.TestOnlyIsInIdleState()); +} + +void TestCommandInteraction::TestCommandHandlerWithoutResponderCallingAddResponse(nlTestSuite * apSuite, void * apContext) +{ + chip::app::ConcreteCommandPath requestCommandPath(kTestEndpointId, kTestClusterId, kTestCommandIdWithData); + CommandHandler commandHandler(&mockCommandHandlerDelegate); + + uint32_t sizeToFill = 50; // This is an arbitrary number, we need to select a non-zero value. + CHIP_ERROR err = commandHandler.AddResponseData(requestCommandPath, ForcedSizeBuffer(sizeToFill)); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + // Since calling AddResponseData is supposed to be a no-operation when there is no responder, it is + // hard to validate. Best way is to check that we are still in an Idle state afterwards + NL_TEST_ASSERT(apSuite, commandHandler.TestOnlyIsInIdleState()); +} + +void TestCommandInteraction::TestCommandHandlerWithoutResponderCallingDirectPrepareFinishCommandApis(nlTestSuite * apSuite, + void * apContext) +{ + chip::app::ConcreteCommandPath requestCommandPath(kTestEndpointId, kTestClusterId, kTestCommandIdWithData); + CommandHandler commandHandler(&mockCommandHandlerDelegate); + + // We intentionally prevent successful calls to PrepareInvokeResponseCommand and FinishCommand when no + // responder is present. This aligns with the design decision to promote AddStatus and AddResponseData + // usage in such scenarios. See GitHub issue #32486 for discussions on phasing out external use of + // these primitives. + const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); + ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; + CHIP_ERROR err = commandHandler.PrepareInvokeResponseCommand(responseCommandPath, prepareParams); + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); + + NL_TEST_ASSERT(apSuite, commandHandler.GetCommandDataIBTLVWriter() == nullptr); + + err = commandHandler.FinishCommand(); + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); + + NL_TEST_ASSERT(apSuite, commandHandler.TestOnlyIsInIdleState()); +} + +void TestCommandInteraction::TestCommandHandlerWithOnInvokeReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); - app::CommandHandler commandHandler(&mockCommandHandlerDelegate); System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); - TestExchangeDelegate delegate; - commandHandler.mResponseSender.SetExchangeContext(ctx.NewExchangeToAlice(&delegate)); // Use some invalid endpoint / cluster / command. GenerateInvokeRequest(apSuite, apContext, commandDatabuf, /* aIsTimedRequest = */ false, 0xEF /* command */, 0xADBE /* cluster */, 0xDE /* endpoint */); - - // TODO: Need to find a way to get the response instead of only check if a function on key path is called. - // We should not reach CommandDispatch if requested command does not exist. + CommandHandler commandHandler(&mockCommandHandlerDelegate); chip::isCommandDispatched = false; - commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), false); + + mockCommandHandlerDelegate.ResetCounter(); + MockCommandResponder mockCommandResponder; + InteractionModel::Status status = commandHandler.OnInvokeCommandRequest(mockCommandResponder, std::move(commandDatabuf), false); + + NL_TEST_ASSERT(apSuite, status == Protocols::InteractionModel::Status::InvalidAction); + NL_TEST_ASSERT(apSuite, mockCommandResponder.mChunks.IsNull()); + // TODO we can further validate the response is what we expected. NL_TEST_ASSERT(apSuite, !chip::isCommandDispatched); } -void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext) +void TestCommandInteraction::TestCommandHandlerWithOnInvokeReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); bool allBooleans[] = { true, false }; for (auto messageIsTimed : allBooleans) { for (auto transactionIsTimed : allBooleans) { - app::CommandHandler commandHandler(&mockCommandHandlerDelegate); + mockCommandHandlerDelegate.ResetCounter(); + CommandHandler commandHandler(&mockCommandHandlerDelegate); System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); - TestExchangeDelegate delegate; - auto exchange = ctx.NewExchangeToAlice(&delegate, false); - commandHandler.mResponseSender.SetExchangeContext(exchange); - chip::isCommandDispatched = false; GenerateInvokeRequest(apSuite, apContext, commandDatabuf, messageIsTimed, kTestCommandIdNoData); + MockCommandResponder mockCommandResponder; Protocols::InteractionModel::Status status = - commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), transactionIsTimed); + commandHandler.OnInvokeCommandRequest(mockCommandResponder, std::move(commandDatabuf), transactionIsTimed); + if (messageIsTimed != transactionIsTimed) { NL_TEST_ASSERT(apSuite, status == Protocols::InteractionModel::Status::TimedRequestMismatch); + NL_TEST_ASSERT(apSuite, mockCommandResponder.mChunks.IsNull()); } else { NL_TEST_ASSERT(apSuite, status == Protocols::InteractionModel::Status::Success); + NL_TEST_ASSERT(apSuite, !mockCommandResponder.mChunks.IsNull()); } NL_TEST_ASSERT(apSuite, chip::isCommandDispatched == (messageIsTimed == transactionIsTimed)); - - // - // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an - // OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest - // above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial - // setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it - // out. This is not expected in normal application logic. - // - exchange->Close(); } } } @@ -1357,7 +1410,7 @@ void TestCommandInteraction::TestCommandSenderLegacyCallbackUnsupportedCommand(n mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 1 && mockCommandSenderDelegate.onErrorCalledTimes == 1); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1381,7 +1434,7 @@ void TestCommandInteraction::TestCommandSenderExtendableCallbackUnsupportedComma mockCommandSenderExtendedDelegate.onFinalCalledTimes == 1 && mockCommandSenderExtendedDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1416,7 +1469,7 @@ void TestCommandInteraction::TestCommandSenderLegacyCallbackBuildingBatchCommand err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1452,7 +1505,7 @@ void TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchDup err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1497,7 +1550,7 @@ void TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchCom err = commandSender.FinishCommand(finishCommandParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1522,7 +1575,7 @@ void TestCommandInteraction::TestCommandSenderCommandSuccessResponseFlow(nlTestS mockCommandSenderDelegate.onErrorCalledTimes == 0); NL_TEST_ASSERT(apSuite, commandSender.GetInvokeResponseMessageCount() == 1); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1546,7 +1599,7 @@ void TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow(nl mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 0 && mockCommandSenderDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 1); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 1); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 2); // Decrease CommandHandler refcount and send response @@ -1558,7 +1611,7 @@ void TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow(nl mockCommandSenderDelegate.onResponseCalledTimes == 1 && mockCommandSenderDelegate.onFinalCalledTimes == 1 && mockCommandSenderDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1581,7 +1634,7 @@ void TestCommandInteraction::TestCommandSenderCommandSpecificResponseFlow(nlTest mockCommandSenderDelegate.onResponseCalledTimes == 1 && mockCommandSenderDelegate.onFinalCalledTimes == 1 && mockCommandSenderDelegate.onErrorCalledTimes == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1606,7 +1659,7 @@ void TestCommandInteraction::TestCommandSenderCommandFailureResponseFlow(nlTestS mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 1 && mockCommandSenderDelegate.onErrorCalledTimes == 1); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1648,7 +1701,7 @@ void TestCommandInteraction::TestCommandSenderAbruptDestruction(nlTestSuite * ap // NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); } void TestCommandInteraction::TestCommandHandlerRejectMultipleIdenticalCommands(nlTestSuite * apSuite, void * apContext) @@ -1696,7 +1749,7 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleIdenticalCommands(n mockCommandSenderExtendedDelegate.onErrorCalledTimes == 1); NL_TEST_ASSERT(apSuite, !chip::isCommandDispatched); - NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, GetNumActiveCommandResponderObjects() == 0); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } @@ -1745,11 +1798,10 @@ void TestCommandInteraction::TestCommandHandlerRejectsMultipleCommandsWithIdenti } } - BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); - TestExchangeDelegate delegate; - auto exchange = ctx.NewExchangeToAlice(&delegate, false); - commandHandler.mResponseSender.SetExchangeContext(exchange); + BasicCommandPathRegistry<4> basicCommandPathRegistry; + MockCommandResponder mockCommandResponder; + CommandHandler::TestOnlyOverrides testOnlyOverrides{ &basicCommandPathRegistry, &mockCommandResponder }; + CommandHandler commandHandler(testOnlyOverrides, &mockCommandHandlerDelegate); // Hackery to steal the InvokeRequest buffer from commandSender. System::PacketBufferHandle commandDatabuf; @@ -1763,15 +1815,6 @@ void TestCommandInteraction::TestCommandHandlerRejectsMultipleCommandsWithIdenti NL_TEST_ASSERT(apSuite, status == InteractionModel::Status::InvalidAction); NL_TEST_ASSERT(apSuite, commandDispatchedCount == 0); - - // - // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an - // OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest - // above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial - // setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it - // out. This is not expected in normal application logic. - // - exchange->Close(); } #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST @@ -1815,11 +1858,6 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandler } } - CommandHandler commandHandler(&mockCommandHandlerDelegate); - TestExchangeDelegate delegate; - auto exchange = ctx.NewExchangeToAlice(&delegate, false); - commandHandler.mResponseSender.SetExchangeContext(exchange); - // Hackery to steal the InvokeRequest buffer from commandSender. System::PacketBufferHandle commandDatabuf; err = commandSender.Finalize(commandDatabuf); @@ -1829,19 +1867,13 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandler sendResponse = true; commandDispatchedCount = 0; - InteractionModel::Status status = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), false); + CommandHandler commandHandler(&mockCommandHandlerDelegate); + MockCommandResponder mockCommandResponder; + InteractionModel::Status status = commandHandler.OnInvokeCommandRequest(mockCommandResponder, std::move(commandDatabuf), false); NL_TEST_ASSERT(apSuite, status == InteractionModel::Status::InvalidAction); + NL_TEST_ASSERT(apSuite, mockCommandResponder.mChunks.IsNull()); NL_TEST_ASSERT(apSuite, commandDispatchedCount == 0); - - // - // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an - // OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest - // above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial - // setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it - // out. This is not expected in normal application logic. - // - exchange->Close(); } void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuite * apSuite, void * apContext) @@ -1884,11 +1916,10 @@ void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuit commandSender.MoveToState(app::CommandSender::State::AddedCommand); } - BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); - TestExchangeDelegate delegate; - auto exchange = ctx.NewExchangeToAlice(&delegate, false); - commandHandler.mResponseSender.SetExchangeContext(exchange); + BasicCommandPathRegistry<4> basicCommandPathRegistry; + MockCommandResponder mockCommandResponder; + CommandHandler::TestOnlyOverrides testOnlyOverrides{ &basicCommandPathRegistry, &mockCommandResponder }; + CommandHandler commandHandler(testOnlyOverrides, &mockCommandHandlerDelegate); // Hackery to steal the InvokeRequest buffer from commandSender. System::PacketBufferHandle commandDatabuf; @@ -1903,29 +1934,23 @@ void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuit NL_TEST_ASSERT(apSuite, status == InteractionModel::Status::Success); NL_TEST_ASSERT(apSuite, commandDispatchedCount == 2); - - // - // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an - // OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest - // above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial - // setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it - // out. This is not expected in normal application logic. - // - exchange->Close(); } void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse( nlTestSuite * apSuite, void * apContext) { - BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + BasicCommandPathRegistry<4> basicCommandPathRegistry; + MockCommandResponder mockCommandResponder; + CommandHandler::TestOnlyOverrides testOnlyOverrides{ &basicCommandPathRegistry, &mockCommandResponder }; + CommandHandler commandHandler(testOnlyOverrides, &mockCommandHandlerDelegate); + commandHandler.mReserveSpaceForMoreChunkMessages = true; ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; - CHIP_ERROR err = mBasicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); + CHIP_ERROR err = basicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = mBasicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); + err = basicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); uint32_t sizeToLeave = 0; @@ -1943,15 +1968,18 @@ void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhere void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative( nlTestSuite * apSuite, void * apContext) { - BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + BasicCommandPathRegistry<4> basicCommandPathRegistry; + MockCommandResponder mockCommandResponder; + CommandHandler::TestOnlyOverrides testOnlyOverrides{ &basicCommandPathRegistry, &mockCommandResponder }; + CommandHandler commandHandler(testOnlyOverrides, &mockCommandHandlerDelegate); + commandHandler.mReserveSpaceForMoreChunkMessages = true; ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; - CHIP_ERROR err = mBasicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); + CHIP_ERROR err = basicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = mBasicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); + err = basicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); uint32_t sizeToLeave = 0; @@ -1969,15 +1997,17 @@ void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhere void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponse(nlTestSuite * apSuite, void * apContext) { - BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + BasicCommandPathRegistry<4> basicCommandPathRegistry; + MockCommandResponder mockCommandResponder; + CommandHandler::TestOnlyOverrides testOnlyOverrides{ &basicCommandPathRegistry, &mockCommandResponder }; + CommandHandler commandHandler(testOnlyOverrides, &mockCommandHandlerDelegate); commandHandler.mReserveSpaceForMoreChunkMessages = true; ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; - CHIP_ERROR err = mBasicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); + CHIP_ERROR err = basicCommandPathRegistry.Add(requestCommandPath1, MakeOptional(static_cast(1))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = mBasicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); + err = basicCommandPathRegistry.Add(requestCommandPath2, MakeOptional(static_cast(2))); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); uint32_t sizeToLeave = 0; @@ -2019,8 +2049,8 @@ void TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed(nlTestS // Mimic closure of the exchange that would happen on a session release and verify that releasing the handle there-after // is handled gracefully. - asyncCommandHandle.Get()->mResponseSender.GetExchangeContext()->GetSessionHolder().Release(); - asyncCommandHandle.Get()->mResponseSender.GetExchangeContext()->OnSessionReleased(); + asyncCommandHandle.Get()->mpResponder->GetExchangeContext()->GetSessionHolder().Release(); + asyncCommandHandle.Get()->mpResponder->GetExchangeContext()->OnSessionReleased(); asyncCommandHandle = nullptr; } @@ -2049,8 +2079,11 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandHandlerCommandEncodeFailure", chip::app::TestCommandInteraction::TestCommandHandlerCommandEncodeFailure), NL_TEST_DEF("TestCommandHandlerCommandEncodeExternalFailure", chip::app::TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure), NL_TEST_DEF("TestCommandHandlerEncodeSimpleStatusCode", chip::app::TestCommandInteraction::TestCommandHandlerEncodeSimpleStatusCode), - NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand), - NL_TEST_DEF("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg), + NL_TEST_DEF("TestCommandHandlerWithoutResponderCallingAddStatus", chip::app::TestCommandInteraction::TestCommandHandlerWithoutResponderCallingAddStatus), + NL_TEST_DEF("TestCommandHandlerWithoutResponderCallingAddResponse", chip::app::TestCommandInteraction::TestCommandHandlerWithoutResponderCallingAddResponse), + NL_TEST_DEF("TestCommandHandlerWithoutResponderCallingDirectPrepareFinishCommandApis", chip::app::TestCommandInteraction::TestCommandHandlerWithoutResponderCallingDirectPrepareFinishCommandApis), + NL_TEST_DEF("TestCommandHandlerWithOnInvokeReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithOnInvokeReceivedNotExistCommand), + NL_TEST_DEF("TestCommandHandlerWithOnInvokeReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithOnInvokeReceivedEmptyDataMsg), NL_TEST_DEF("TestCommandHandlerRejectMultipleIdenticalCommands", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleIdenticalCommands), #if CONFIG_BUILD_FOR_HOST_UNIT_TEST NL_TEST_DEF("TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef", chip::app::TestCommandInteraction::TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef),