Refactor CommandHandler to be more unittestable

.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 \

src/app/BUILD.gn

@@ -271,9 +271,9 @@ static_library("app") {
     "ChunkedWriteCallback.cpp",
     "ChunkedWriteCallback.h",
     "CommandHandler.cpp",
+    "CommandResponderInterface.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"

src/app/CommandResponderInterface.h

@@ -0,0 +1,64 @@
+/*
+ *    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 <lib/core/Optional.h>
+#include <messaging/ExchangeHolder.h>
+#include <protocols/interaction_model/StatusCode.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 CommandResponder behavior. For example, it can stub out sending a response.
+ */
+class CommandResponderInterface
+{
+public:
+    virtual ~CommandResponderInterface() = default;
+
+    /* ExchangeContext related getter methods */
+    virtual Messaging::ExchangeContext * GetExchangeContext() const = 0;
+    virtual Access::SubjectDescriptor GetSubjectDescriptor() const  = 0;
+    virtual FabricIndex GetAccessingFabricIndex() const             = 0;
+    virtual Optional<GroupId> GetGroupId() const                    = 0;
+
+    /**
+     * @brief Flush acks right now.
+     *
+     * Typically called when processing a slow command.
+     */
+    virtual void FlushAcksRightNow() = 0;
+
+    /**
+     * @brief Adds completed InvokeResponseMessage for sending to command requestor.
+     */
+    virtual void AddInvokeResponseToSend(System::PacketBufferHandle && aPacket) = 0;
+    /**
+     * @brief Called to indicate that response was dropped.
+     */
+    virtual void ResponseDropped() = 0;
+};
+
+} // namespace app
+} // namespace chip

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 still close the exchange. Since sending an
+        // InvokeResponseMessage seems problematic, we'll send a StatusResponse with 'Failure'
+        // instead.
         VerifyOrExit(err == CHIP_NO_ERROR, failureStatusToSend.SetValue(Status::Failure));
 
         bool moreToSend = !mChunks.IsNull();
@@ -78,16 +76,33 @@ 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();
 }
 
-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 +113,6 @@ CHIP_ERROR CommandResponseSender::StartSendingCommandResponses()
     {
         Close();
     }
-    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR CommandResponseSender::SendCommandResponse()
@@ -157,12 +171,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 InvokeCommand Request message because we explicitly registered with the
+    // Exchange Manager for unsolicited InvokeCommand Requests.
+    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.
+    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.
+        mDelayCallingCloseUntilOnDone = true;
     }
 }
 
+#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