Auto-close exchanges on response timeout, unless consumer says otherw…

…ise. (#8350)

Unless the consumer sends another message or says to keep waiting, the
exchange should close.

examples/shell/shell_common/cmd_send.cpp

@@ -119,7 +119,6 @@ class MockAppDelegate : public Messaging::ExchangeDelegate
         streamer_t * sout = streamer_get();
         streamer_printf(sout, "No response received\n");
 
-        gExchangeCtx->Close();
         gExchangeCtx = nullptr;
     }
 } gMockAppDelegate;

src/app/Command.cpp

@@ -129,11 +129,16 @@ CHIP_ERROR Command::ProcessCommandMessage(System::PacketBufferHandle && payload,
 void Command::Shutdown()
 {
     VerifyOrReturn(mState != CommandState::Uninitialized);
-    mCommandMessageWriter.Reset();
-
     AbortExistingExchangeContext();
+    ShutdownInternal();
+}
+
+void Command::ShutdownInternal()
+{
+    mCommandMessageWriter.Reset();
 
     mpExchangeMgr = nullptr;
+    mpExchangeCtx = nullptr;
     mpDelegate    = nullptr;
     ClearState();
 

src/app/Command.h

@@ -78,9 +78,8 @@ class Command
     CHIP_ERROR Init(Messaging::ExchangeManager * apExchangeMgr, InteractionModelDelegate * apDelegate);
 
     /**
-     *  Shutdown the CommandSender. This terminates this instance
+     *  Shutdown the Command. This terminates this instance
      *  of the object and releases all held resources.
-     *
      */
     void Shutdown();
 
@@ -126,6 +125,12 @@ class Command
     void ClearState();
     const char * GetStateStr() const;
 
+    /**
+     * Internal shutdown method that we use when we know what's going on with
+     * our exchange and don't need to manually close it.
+     */
+    void ShutdownInternal();
+
     InvokeCommand::Builder mInvokeCommandBuilder;
     Messaging::ExchangeManager * mpExchangeMgr = nullptr;
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;

src/app/CommandHandler.cpp

@@ -72,9 +72,7 @@ CHIP_ERROR CommandHandler::SendCommandResponse()
     MoveToState(CommandState::Sending);
 
 exit:
-    // Keep Shutdown() from double-closing our exchange.
-    mpExchangeCtx = nullptr;
-    Shutdown();
+    ShutdownInternal();
     ChipLogFunctError(err);
     return err;
 }

src/app/CommandSender.cpp

@@ -92,10 +92,6 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
 exit:
     ChipLogFunctError(err);
 
-    // Null out mpExchangeCtx, so our Shutdown() call below won't try to abort
-    // it and fail to send an ack for the message we just received.
-    mpExchangeCtx = nullptr;
-
     if (mpDelegate != nullptr)
     {
         if (err != CHIP_NO_ERROR)
@@ -108,7 +104,7 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
         }
     }
 
-    Shutdown();
+    ShutdownInternal();
     return err;
 }
 
@@ -122,7 +118,7 @@ void CommandSender::OnResponseTimeout(Messaging::ExchangeContext * apExchangeCon
         mpDelegate->CommandResponseError(this, CHIP_ERROR_TIMEOUT);
     }
 
-    Shutdown();
+    ShutdownInternal();
 }
 
 CHIP_ERROR CommandSender::ProcessCommandDataElement(CommandDataElement::Parser & aCommandElement)

src/app/ReadClient.cpp

@@ -52,7 +52,13 @@ CHIP_ERROR ReadClient::Init(Messaging::ExchangeManager * apExchangeMgr, Interact
 void ReadClient::Shutdown()
 {
     AbortExistingExchangeContext();
+    ShutdownInternal();
+}
+
+void ReadClient::ShutdownInternal()
+{
     mpExchangeMgr = nullptr;
+    mpExchangeCtx = nullptr;
     mpDelegate    = nullptr;
     MoveToState(ClientState::Uninitialized);
 }
@@ -229,9 +235,6 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
 exit:
     ChipLogFunctError(err);
 
-    // Null out mpExchangeCtx, so our Shutdown() call below won't try to abort
-    // it and fail to send an ack for the message we just received.
-    mpExchangeCtx = nullptr;
     MoveToState(ClientState::Initialized);
 
     if (mpDelegate != nullptr)
@@ -247,7 +250,7 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
     }
 
     // TODO(#7521): Should close it after checking moreChunkedMessages flag is not set.
-    Shutdown();
+    ShutdownInternal();
 
     return err;
 }
@@ -351,7 +354,7 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex
     {
         mpDelegate->ReportError(this, CHIP_ERROR_TIMEOUT);
     }
-    Shutdown();
+    ShutdownInternal();
 }
 
 CHIP_ERROR ReadClient::ProcessAttributeDataList(TLV::TLVReader & aAttributeDataListReader)

src/app/ReadClient.h

@@ -56,7 +56,10 @@ class ReadClient : public Messaging::ExchangeDelegate
      *  all held resources.  The object must not be used after Shutdown() is called.
      *
      *  SDK consumer can choose when to shut down the ReadClient.
-     *  The ReadClient will never shut itself down, unless the overall InteractionModelEngine is shut down.
+     *  The ReadClient will automatically shut itself down when it receives a
+     *  response or the response times out.  So manual shutdown is only needed
+     *  to shut down a ReadClient before one of those two things has happened,
+     *  (e.g. if SendReadRequest returned failure).
      */
     void Shutdown();
 
@@ -127,6 +130,12 @@ class ReadClient : public Messaging::ExchangeDelegate
     CHIP_ERROR AbortExistingExchangeContext();
     const char * GetStateStr() const;
 
+    /**
+     * Internal shutdown method that we use when we know what's going on with
+     * our exchange and don't need to manually close it.
+     */
+    void ShutdownInternal();
+
     Messaging::ExchangeManager * mpExchangeMgr = nullptr;
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;
     InteractionModelDelegate * mpDelegate      = nullptr;

src/app/WriteClient.cpp

@@ -58,11 +58,16 @@ CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Interac
 void WriteClient::Shutdown()
 {
     VerifyOrReturn(mState != State::Uninitialized);
-    mMessageWriter.Reset();
-
     ClearExistingExchangeContext();
+    ShutdownInternal();
+}
+
+void WriteClient::ShutdownInternal()
+{
+    mMessageWriter.Reset();
 
     mpExchangeMgr         = nullptr;
+    mpExchangeCtx         = nullptr;
     mpDelegate            = nullptr;
     mAttributeStatusIndex = 0;
     ClearState();
@@ -294,9 +299,6 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
 
     VerifyOrDie(apExchangeContext == mpExchangeCtx);
 
-    // We are done with this exchange, and it will be closing itself.
-    mpExchangeCtx = nullptr;
-
     // Verify that the message is an Write Response.
     // If not, close the exchange and free the payload.
     if (!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteResponse))
@@ -318,7 +320,7 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
             mpDelegate->WriteResponseProcessed(this);
         }
     }
-    Shutdown();
+    ShutdownInternal();
     return err;
 }
 
@@ -331,7 +333,7 @@ void WriteClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeConte
     {
         mpDelegate->WriteResponseError(this, CHIP_ERROR_TIMEOUT);
     }
-    Shutdown();
+    ShutdownInternal();
 }
 
 CHIP_ERROR WriteClient::ProcessAttributeStatusElement(AttributeStatusElement::Parser & aAttributeStatusElement)

src/app/WriteClient.h

@@ -115,6 +115,12 @@ class WriteClient : public Messaging::ExchangeDelegate
     const char * GetStateStr() const;
     void ClearState();
 
+    /**
+     * Internal shutdown method that we use when we know what's going on with
+     * our exchange and don't need to manually close it.
+     */
+    void ShutdownInternal();
+
     Messaging::ExchangeManager * mpExchangeMgr = nullptr;
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;
     InteractionModelDelegate * mpDelegate      = nullptr;

src/controller/CHIPDevice.cpp

@@ -339,10 +339,7 @@ CHIP_ERROR Device::OnMessageReceived(Messaging::ExchangeContext * exchange, cons
     return CHIP_NO_ERROR;
 }
 
-void Device::OnResponseTimeout(Messaging::ExchangeContext * ec)
-{
-    ec->Close();
-}
+void Device::OnResponseTimeout(Messaging::ExchangeContext * ec) {}
 
 CHIP_ERROR Device::OpenPairingWindow(uint32_t timeout, PairingWindowOption option, SetupPayload & setupPayload)
 {

src/messaging/ExchangeContext.cpp

@@ -363,21 +363,22 @@ void ExchangeContext::HandleResponseTimeout(System::Layer * aSystemLayer, void *
     if (ec == nullptr)
         return;
 
-    // NOTE: we don't set mResponseExpected to false here because the response could still arrive. If the user
-    // wants to never receive the response, they must close the exchange context.
-
     ec->NotifyResponseTimeout();
 }
 
 void ExchangeContext::NotifyResponseTimeout()
 {
+    SetResponseExpected(false);
+
     ExchangeDelegate * delegate = GetDelegate();
 
     // Call the user's timeout handler.
     if (delegate != nullptr)
     {
         delegate->OnResponseTimeout(this);
     }
+
+    MessageHandled();
 }
 
 CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,

src/messaging/ExchangeDelegate.h

@@ -75,6 +75,16 @@ class DLL_EXPORT ExchangeDelegate
      *   This function is the protocol callback to invoke when the timeout for the receipt
      *   of a response message has expired.
      *
+     *   After calling this method an exchange will close itself unless one of
+     *   two things happens:
+     *
+     *   1) A call to SendMessage on the exchange with the kExpectResponse flag
+     *      set.
+     *   2) A call to WillSendMessage on the exchange.
+     *
+     *   Consumers that don't do one of those things MUST NOT retain a pointer
+     *   to the exchange.
+     *
      *  @param[in]    ec            A pointer to the ExchangeContext object.
      */
     virtual void OnResponseTimeout(ExchangeContext * ec) = 0;

src/messaging/tests/TestExchangeMgr.cpp

@@ -84,11 +84,7 @@ class WaitForTimeoutDelegate : public ExchangeDelegate
         return CHIP_NO_ERROR;
     }
 
-    void OnResponseTimeout(ExchangeContext * ec) override
-    {
-        IsOnResponseTimeoutCalled = true;
-        ec->Close();
-    }
+    void OnResponseTimeout(ExchangeContext * ec) override { IsOnResponseTimeoutCalled = true; }
 
     bool IsOnResponseTimeoutCalled = false;
 };

src/protocols/secure_channel/CASESession.cpp

@@ -303,6 +303,9 @@ void CASESession::OnResponseTimeout(ExchangeContext * ec)
                  "CASESession timed out while waiting for a response from the peer. Expected message type was %" PRIu8,
                  to_underlying(mNextExpectedMsg));
     mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
+    // Null out mExchangeCtxt so that Clear() doesn't try closing it.  The
+    // exchange will handle that.
+    mExchangeCtxt = nullptr;
     Clear();
 }
 

src/protocols/secure_channel/MessageCounterManager.cpp

@@ -112,8 +112,6 @@ void MessageCounterManager::OnResponseTimeout(Messaging::ExchangeContext * excha
     {
         ChipLogError(SecureChannel, "Timed out! Failed to clear message counter synchronization status.");
     }
-
-    exchangeContext->Close();
 }
 
 CHIP_ERROR MessageCounterManager::AddToReceiveTable(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress,

src/protocols/secure_channel/PASESession.cpp

@@ -333,6 +333,9 @@ void PASESession::OnResponseTimeout(ExchangeContext * ec)
                  "PASESession timed out while waiting for a response from the peer. Expected message type was %" PRIu8,
                  to_underlying(mNextExpectedMsg));
     mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
+    // Null out mExchangeCtxt so that Clear() doesn't try closing it.  The
+    // exchange will handle that.
+    mExchangeCtxt = nullptr;
     Clear();
 }