Address PR comments

Author: tehampson

src/app/BUILD.gn

@@ -271,7 +271,7 @@ static_library("app") {
     "ChunkedWriteCallback.cpp",
     "ChunkedWriteCallback.h",
     "CommandHandler.cpp",
-    "CommandResponderInterface.h",
+    "CommandHandlerExchangeInterface.h",
     "CommandResponseHelper.h",
     "CommandResponseSender.cpp",
     "DefaultAttributePersistenceProvider.cpp",

src/app/CommandHandler.cpp

@@ -99,7 +99,7 @@ CHIP_ERROR CommandHandler::AllocateBuffer()
     return CHIP_NO_ERROR;
 }
 
-Status CommandHandler::OnInvokeCommandRequest(CommandResponderInterface & commandResponder, System::PacketBufferHandle && payload,
+Status CommandHandler::OnInvokeCommandRequest(CommandHandlerExchangeInterface & commandResponder, System::PacketBufferHandle && payload,
                                               bool isTimedInvoke)
 {
     VerifyOrDieWithMsg(mState == State::Idle, DataManagement, "state should be Idle");
@@ -845,7 +845,7 @@ CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessage(bool aHasMoreChunks)
     return CHIP_NO_ERROR;
 }
 
-void CommandHandler::SetCommandResponder(CommandResponderInterface * commandResponder)
+void CommandHandler::SetCommandResponder(CommandHandlerExchangeInterface * commandResponder)
 {
     VerifyOrDieWithMsg(mState == State::Idle, DataManagement, "CommandResponseSender can only be set in idle state");
     mpResponder = commandResponder;
@@ -923,7 +923,7 @@ 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(CommandResponderInterface & commandResponder,
+void CommandHandler::TestOnlyInvokeCommandRequestWithFaultsInjected(CommandHandlerExchangeInterface & commandResponder,
                                                                     System::PacketBufferHandle && payload, bool isTimedInvoke,
                                                                     NlFaultInjectionType faultType)
 {

src/app/CommandHandler.h

@@ -32,7 +32,7 @@
 
 #include "CommandPathRegistry.h"
 
-#include <app/CommandResponderInterface.h>
+#include <app/CommandHandlerExchangeInterface.h>
 #include <app/ConcreteCommandPath.h>
 #include <app/data-model/Encode.h>
 #include <lib/core/CHIPCore.h>
@@ -181,7 +181,7 @@ class CommandHandler
     {
     public:
         CommandPathRegistry * commandPathRegistry    = nullptr;
-        CommandResponderInterface * commandResponder = nullptr;
+        CommandHandlerExchangeInterface * commandResponder = nullptr;
     };
 
     /*
@@ -216,7 +216,7 @@ class CommandHandler
      * command responder object must outlive this CommandHandler object. It is only safe to
      * release after client recieved OnDone callback.
      */
-    Protocols::InteractionModel::Status OnInvokeCommandRequest(CommandResponderInterface & commandResponder,
+    Protocols::InteractionModel::Status OnInvokeCommandRequest(CommandHandlerExchangeInterface & commandResponder,
                                                                System::PacketBufferHandle && payload, bool isTimedInvoke);
 
     /**
@@ -386,7 +386,7 @@ class CommandHandler
     {
         if (mpResponder)
         {
-            mpResponder->FlushAcksRightNow();
+            mpResponder->HandlingSlowCommand();
         }
     }
 
@@ -428,7 +428,7 @@ class CommandHandler
      */
     // TODO(#30453): After refactoring CommandHandler for better unit testability, create a
     // unit test specifically for the fault injection behavior.
-    void TestOnlyInvokeCommandRequestWithFaultsInjected(CommandResponderInterface & commandResponder,
+    void TestOnlyInvokeCommandRequestWithFaultsInjected(CommandHandlerExchangeInterface & commandResponder,
                                                         System::PacketBufferHandle && payload, bool isTimedInvoke,
                                                         NlFaultInjectionType faultType);
 #endif // CHIP_WITH_NLFAULTINJECTION
@@ -654,7 +654,7 @@ class CommandHandler
         return FinishCommand(/* aEndDataStruct = */ false);
     }
 
-    void SetCommandResponder(CommandResponderInterface * commandResponder);
+    void SetCommandResponder(CommandHandlerExchangeInterface * commandResponder);
 
     /**
      * Check whether the InvokeRequest we are handling is targeted to a group.
@@ -688,7 +688,7 @@ class CommandHandler
     CommandPathRegistry * mCommandPathRegistry = &mBasicCommandPathRegistry;
     Optional<uint16_t> mRefForResponse;
 
-    CommandResponderInterface * mpResponder = nullptr;
+    CommandHandlerExchangeInterface * mpResponder = nullptr;
 
     State mState = State::Idle;
     State mBackupState;

src/app/CommandHandlerExchangeInterface.h

@@ -0,0 +1,105 @@
+/*
+ *    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 <access/SubjectDescriptor.h>
+#include <lib/core/DataModelTypes.h>
+#include <lib/core/GroupId.h>
+#include <lib/core/Optional.h>
+#include <messaging/ExchangeContext.h>
+#include <protocols/interaction_model/StatusCode.h>
+#include <system/SystemPacketBuffer.h>
+
+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 `emberAf...` methods without intending to sending a response on the exchange.
+ * These ember methods require providing an instance of CommandHandler where a status response
+ * is added (see github.com/project-chip/connectedhomeip/issues/32030).
+ */
+class CommandHandlerExchangeInterface
+{
+public:
+    virtual ~CommandHandlerExchangeInterface() = default;
+
+    /**
+     * Gets the inner exchange context object associated with incoming
+     * InvokeRequestMessage, without ownership.
+     *
+     * @return The inner exchange context, might be nullptr if no
+     *         exchange context has been assigned or the context
+     *         has been released.
+     */
+    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 the incoming session is a group session, returns its group ID. Otherwise,
+     * returns a null optional.
+     */
+    virtual Optional<GroupId> GetGroupId() const                    = 0;
+
+    /**
+     * @brief Called to indicates slow command is being processed.
+     *
+     * Facilitates communication on the exchange (if needed) that slow command is being
+     * processed. For example, sending a standalone ack when using MRP.
+     */
+    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

src/app/CommandResponderInterface.h

@@ -39,9 +39,9 @@ CHIP_ERROR CommandResponseSender::OnMessageReceived(Messaging::ExchangeContext *
         VerifyOrExit(err == CHIP_NO_ERROR, failureStatusToSend.SetValue(Status::InvalidAction));
 
         err = SendCommandResponse();
-        // If SendCommandResponse() fails, we must still close the exchange. Since sending an
-        // InvokeResponseMessage seems problematic, we'll send a StatusResponse with 'Failure'
-        // instead.
+        // 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();
@@ -76,8 +76,6 @@ void CommandResponseSender::OnResponseTimeout(Messaging::ExchangeContext * apExc
 {
     ChipLogDetail(DataManagement, "CommandResponseSender: Timed out waiting for response from requester mState=[%10.10s]",
                   GetStateStr());
-    // We don't need to consider remaining async CommandHandlers here. We become the exchange
-    // delegate only after CommandHandler::OnDone is called.
     Close();
 }
 
@@ -99,9 +97,9 @@ void CommandResponseSender::StartSendingCommandResponses()
         //   {
         //       SendStatusResponse(Status::Failure);
         //   }
-        //   Close();
-        //   return;
         //   ```
+        Close();
+        return;
     }
 
     if (HasMoreToSend())
@@ -115,6 +113,31 @@ void CommandResponseSender::StartSendingCommandResponses()
     }
 }
 
+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()
 {
     VerifyOrReturnError(HasMoreToSend(), CHIP_ERROR_INCORRECT_STATE);
@@ -153,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";
@@ -185,8 +211,8 @@ void CommandResponseSender::OnInvokeCommandRequest(Messaging::ExchangeContext *
     mExchangeCtx.Grab(ec);
     mExchangeCtx->WillSendMessage();
 
-    // Grabbing Handle to prevent OnDone from being called before OnInvokeCommandRequest returns.
-    // This allows us to send a StatusResponse error instead of the queued up InvokeResponseMessages.
+    // 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)
@@ -196,7 +222,7 @@ void CommandResponseSender::OnInvokeCommandRequest(Messaging::ExchangeContext *
         // 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.
-        mDelayCallingCloseUntilOnDone = true;
+        MoveToState(State::ErrorSentDelayCloseUntilOnDone);
     }
 }