diff --git a/examples/shell/shell_common/cmd_send.cpp b/examples/shell/shell_common/cmd_send.cpp index e97b45ad6ce5c4..c7e034b4841172 100644 --- a/examples/shell/shell_common/cmd_send.cpp +++ b/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; diff --git a/src/app/Command.cpp b/src/app/Command.cpp index 6edab55606c8a8..8569d4c260484f 100644 --- a/src/app/Command.cpp +++ b/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(); diff --git a/src/app/Command.h b/src/app/Command.h index 82931fb7f3a957..89e5b3a2f0d9f5 100644 --- a/src/app/Command.h +++ b/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; diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 20e3d8cd9776a3..4e5bc825319e0f 100644 --- a/src/app/CommandHandler.cpp +++ b/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; } diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 8764ef842818a6..0dbe74d8e8411b 100644 --- a/src/app/CommandSender.cpp +++ b/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) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index e68342a808b2e0..d68787b80a44b2 100644 --- a/src/app/ReadClient.cpp +++ b/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) diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 7b87d1d7a42e0e..6aa52625866879 100644 --- a/src/app/ReadClient.h +++ b/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; diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index c64c147205c34c..19fdb409865144 100644 --- a/src/app/WriteClient.cpp +++ b/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) diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h index c150722f97300d..8606647c601b87 100644 --- a/src/app/WriteClient.h +++ b/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; diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index ab473927a42224..7cc4e4e2eb8553 100644 --- a/src/controller/CHIPDevice.cpp +++ b/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) { diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 5a183272d719df..2970cf5ddfa43a 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -363,14 +363,13 @@ 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. @@ -378,6 +377,8 @@ void ExchangeContext::NotifyResponseTimeout() { delegate->OnResponseTimeout(this); } + + MessageHandled(); } CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, diff --git a/src/messaging/ExchangeDelegate.h b/src/messaging/ExchangeDelegate.h index db694dd477b3e2..cc28e9b836f546 100644 --- a/src/messaging/ExchangeDelegate.h +++ b/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; diff --git a/src/messaging/tests/TestExchangeMgr.cpp b/src/messaging/tests/TestExchangeMgr.cpp index 2964d6f7e798b9..aa635c69e34468 100644 --- a/src/messaging/tests/TestExchangeMgr.cpp +++ b/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; }; diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 5d84bf464d81bf..021df79b26587a 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/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(); } diff --git a/src/protocols/secure_channel/MessageCounterManager.cpp b/src/protocols/secure_channel/MessageCounterManager.cpp index cd318ad8b58df7..62fbf8e2ae355a 100644 --- a/src/protocols/secure_channel/MessageCounterManager.cpp +++ b/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, diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index b98b79fe7aa13d..e2a1af6d587c3a 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/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(); }