From 3731200792c44cc620d9ef074f1b722ec2bf82a8 Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Mon, 29 Aug 2022 01:14:05 -0400 Subject: [PATCH] Fix ExchangeContext leaks (#21846) --- src/app/tests/TestCommandInteraction.cpp | 92 +++- src/app/tests/TestReadInteraction.cpp | 6 +- src/messaging/ExchangeContext.cpp | 78 +-- src/messaging/ExchangeContext.h | 17 + src/messaging/ExchangeDelegate.h | 12 +- src/messaging/ExchangeHolder.h | 65 ++- src/messaging/ExchangeMgr.cpp | 4 +- src/messaging/ExchangeMgr.h | 12 +- src/messaging/tests/BUILD.gn | 6 +- src/messaging/tests/MessagingContext.cpp | 8 +- src/messaging/tests/MessagingContext.h | 4 +- src/messaging/tests/TestExchangeHolder.cpp | 484 ++++++++++++++++-- .../tests/LoopbackTransportManager.h | 1 + 13 files changed, 677 insertions(+), 112 deletions(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 265d982acbaae4..fde62b54ecd3bd 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -245,7 +245,10 @@ class TestCommandInteraction static void TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerRejectMultipleCommands(nlTestSuite * apSuite, void * apContext); + +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST static void TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext); +#endif static void TestCommandSenderCommandSuccessResponseFlow(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderCommandAsyncSuccessResponseFlow(nlTestSuite * apSuite, void * apContext); @@ -496,10 +499,21 @@ void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSu NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); TestExchangeDelegate delegate; - commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate)); + + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); + err = commandHandler.SendCommandResponse(); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); + + // + // 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(); } void TestCommandInteraction::TestCommandSenderWithSendCommand(nlTestSuite * apSuite, void * apContext) @@ -532,7 +546,8 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); TestExchangeDelegate delegate; - commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate)); + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); err = commandHandler.PrepareCommand(path); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -540,6 +555,7 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); err = commandHandler.SendCommandResponse(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + commandHandler.Close(); } @@ -565,7 +581,8 @@ void TestCommandInteraction::ValidateCommandHandlerWithSendCommand(nlTestSuite * System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; - commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate)); + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); AddInvokeResponseData(apSuite, apContext, &commandHandler, aNeedStatusCode); err = commandHandler.Finalize(commandPacket); @@ -580,6 +597,14 @@ void TestCommandInteraction::ValidateCommandHandlerWithSendCommand(nlTestSuite * err = invokeResponseMessageParser.CheckSchemaValidity(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); #endif + + // + // 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(); } void TestCommandInteraction::TestCommandHandlerWithSendSimpleCommandData(nlTestSuite * apSuite, void * apContext) @@ -625,7 +650,8 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; - commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate)); + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); auto path = MakeTestCommandPath(); @@ -642,6 +668,14 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * err = invokeResponseMessageParser.CheckSchemaValidity(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); #endif + + // + // 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(); } void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext) @@ -652,7 +686,8 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; - commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate)); + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); auto path = MakeTestCommandPath(); @@ -669,6 +704,14 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite err = invokeResponseMessageParser.CheckSchemaValidity(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); #endif + + // + // 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(); } // Command Sender sends invoke request, command handler drops invoke response, then test injects status response message with @@ -1001,7 +1044,8 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTe System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; - commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate)); + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); auto path = MakeTestCommandPath(); @@ -1022,6 +1066,14 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTe err = invokeResponseMessageParser.CheckSchemaValidity(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); #endif + + // + // 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(); } void TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext) @@ -1060,7 +1112,8 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(n System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); TestExchangeDelegate delegate; - commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate)); + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); chip::isCommandDispatched = false; GenerateInvokeRequest(apSuite, apContext, commandDatabuf, messageIsTimed, kTestCommandIdNoData); @@ -1075,6 +1128,15 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(n NL_TEST_ASSERT(apSuite, status == Protocols::InteractionModel::Status::Success); } 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(); } } } @@ -1284,6 +1346,11 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommands(nlTestSuit NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST +// +// This test needs a special unit-test only API being exposed in ExchangeContext to be able to correctly simulate +// the release of a session on the exchange. +// void TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -1303,10 +1370,14 @@ void TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed(nlTestS // Verify that async command handle has been allocated NL_TEST_ASSERT(apSuite, asyncCommandHandle.Get() != nullptr); - // Close the exchange associated with the handle and verify the handle can be gracefully released - asyncCommandHandle.Get()->mExchangeCtx->Close(); + // Mimick closure of the exchange that would happen on a session release and verify that releasing the handle there-after + // is handled gracefully. + asyncCommandHandle.Get()->mExchangeCtx->GetSessionHolder().Release(); + asyncCommandHandle.Get()->mExchangeCtx->OnSessionReleased(); + asyncCommandHandle = nullptr; } +#endif } // namespace app } // namespace chip @@ -1332,7 +1403,10 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand), NL_TEST_DEF("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg), NL_TEST_DEF("TestCommandHandlerRejectMultipleCommands", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleCommands), + +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST NL_TEST_DEF("TestCommandHandlerReleaseWithExchangeClosed", chip::app::TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed), +#endif NL_TEST_DEF("TestCommandSenderCommandSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandSuccessResponseFlow), NL_TEST_DEF("TestCommandSenderCommandAsyncSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow), diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index f96f5279e75023..58d81f0e3064c3 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -492,7 +492,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); { - Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr); + Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr, false); ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read); GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/); @@ -635,7 +635,7 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); { - Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr); + Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr, false); ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read); GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/); @@ -1405,7 +1405,7 @@ void TestReadInteraction::TestProcessSubscribeRequest(nlTestSuite * apSuite, voi err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr); + Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr, false); { ReadHandler readHandler(*engine, exchangeCtx, chip::app::ReadHandler::InteractionType::Read); diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 7bd0aaffa79025..9d3c8ec9a2f401 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -139,13 +139,6 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp bool isStandaloneAck = (protocolId == Protocols::SecureChannel::Id) && msgType == to_underlying(Protocols::SecureChannel::MsgType::StandaloneAck); - if (!isStandaloneAck) - { - // If we were waiting for a message send, this is it. Standalone acks - // are not application-level sends, which is why we don't allow those to - // clear the WillSendMessage flag. - mFlags.Clear(Flags::kFlagWillSendMessage); - } VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INTERNAL); VerifyOrReturnError(mSession, CHIP_ERROR_CONNECTION_ABORTED); @@ -208,10 +201,23 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp return CHIP_ERROR_MISSING_SECURE_SESSION; } - // Create a new scope for `err`, to avoid shadowing warning previous `err`. - CHIP_ERROR err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get().Value(), mExchangeId, - IsInitiator(), GetReliableMessageContext(), reliableTransmissionRequested, - protocolId, msgType, std::move(msgBuf)); + CHIP_ERROR err; + +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST + if (mInjectedFailures.Has(InjectedFailureType::kFailOnSend)) + { + err = CHIP_ERROR_SENDING_BLOCKED; + } + else + { +#endif + err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get().Value(), mExchangeId, IsInitiator(), + GetReliableMessageContext(), reliableTransmissionRequested, protocolId, msgType, + std::move(msgBuf)); +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST + } +#endif + if (err != CHIP_NO_ERROR && IsResponseExpected()) { CancelResponseTimer(); @@ -219,8 +225,12 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp } // Standalone acks are not application-level message sends. - if (err == CHIP_NO_ERROR && !isStandaloneAck) + if (!isStandaloneAck && err == CHIP_NO_ERROR) { + // + // Once we've sent the message successfully, we can clear out the WillSendMessage flag. + // + mFlags.Clear(Flags::kFlagWillSendMessage); MessageHandled(); } @@ -230,6 +240,11 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp void ExchangeContext::DoClose(bool clearRetransTable) { + if (mFlags.Has(Flags::kFlagClosed)) + { + return; + } + mFlags.Set(Flags::kFlagClosed); // Clear protocol callbacks @@ -336,7 +351,11 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons ExchangeContext::~ExchangeContext() { VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0); - VerifyOrDie(!IsAckPending()); + + // + // Ensure that DoClose has been called by the time we get here. If not, we have a leak somewhere. + // + VerifyOrDie(mFlags.Has(Flags::kFlagClosed)); #if CONFIG_DEVICE_LAYER && CHIP_DEVICE_CONFIG_ENABLE_SED // Make sure that the exchange withdraws the request for Sleepy End Device active mode. @@ -398,28 +417,27 @@ void ExchangeContext::OnSessionReleased() // decrease our refcount without worrying about use-after-free. ExchangeHandle ref(*this); - if (IsResponseExpected()) + // + // If a send is not expected (either because we're waiting for a response OR + // we're in the middle of processing a OnMessageReceived call), we can go ahead + // and notify our delegate and abort the exchange since we still own the ref. + // + if (!IsSendExpected()) { - // If we're waiting on a response, we now know it's never going to show up - // and we should notify our delegate accordingly. - CancelResponseTimer(); - // We want to Abort, not just Close, so that RMP bits are cleared, so - // don't let NotifyResponseTimeout close us. - NotifyResponseTimeout(/* aCloseIfNeeded = */ false); + if (IsResponseExpected()) + { + // If we're waiting on a response, we now know it's never going to show up + // and we should notify our delegate accordingly. + CancelResponseTimer(); + // We want to Abort, not just Close, so that RMP bits are cleared, so + // don't let NotifyResponseTimeout close us. + NotifyResponseTimeout(/* aCloseIfNeeded = */ false); + } + Abort(); } else { - // Either we're expecting a send or we are in our "just allocated, first - // send has not happened yet" state. - // - // Just mark ourselves as closed. The consumer is responsible for - // releasing us. See documentation for - // ExchangeDelegate::OnExchangeClosing. - if (IsSendExpected()) - { - mFlags.Clear(Flags::kFlagWillSendMessage); - } DoClose(true /* clearRetransTable */); } } diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index 3e7e56121c6d26..4631cb0e0b81ad 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -212,7 +212,24 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, */ bool IsSendExpected() const { return mFlags.Has(Flags::kFlagWillSendMessage); } +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST + SessionHolder & GetSessionHolder() { return mSession; } + + enum class InjectedFailureType : uint8_t + { + kFailOnSend = 0x01 + }; + + void InjectFailure(InjectedFailureType failureType) { mInjectedFailures.Set(failureType); } + + void ClearInjectedFailures() { mInjectedFailures.ClearAll(); } +#endif + private: +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST + BitFlags mInjectedFailures; +#endif + class ExchangeSessionHolder : public SessionHolderWithDelegate { public: diff --git a/src/messaging/ExchangeDelegate.h b/src/messaging/ExchangeDelegate.h index 363f0508d34515..97a5a0adaebb42 100644 --- a/src/messaging/ExchangeDelegate.h +++ b/src/messaging/ExchangeDelegate.h @@ -61,13 +61,13 @@ class ExchangeContext; * on the exchange, then the exchange remains with you, and it's your responsibility to either send a message on it, * or Close/Abort if you no longer wish to have the exchange around. * - * 6. If you get a call to OnExchangeClosing, you should give up your reference to the exchange - * by 'nulling' out your reference to the exchange. The exchange will be automatically closed by the ExchangeMgr. + * 6. If you get a call to OnExchangeClosing, you should null out your reference to the exchange UNLESS you still + * hold ownership of the exchange (i.e due to a prior call to WillSendMessage). In that case, you should call Abort/Close + * whenever you're done with using the exchange. Those calls can be made synchronously within the OnExchangeClosing + * callback. * - * 6. If you get a call to OnResponseTimeout, you should give up your reference to the exchange - * by 'nulling' out your reference to the exchange UNLESS you intend to do further work on the exchange. If so, - * rules 2, 3 and 5 apply. Otherwise, the exchange will be automatically closed by the ExchangeMgr. Note that - * if the cause of the call is the release of the underlying session, attempts to send a message will result in failure. + * 7. If you get a call to OnResponseTimeout, you should null out your reference to the exchange since the exchange layer + * owns the exchange and will handle releasing the ref later. A call to OnExchangeClosing will follow after. * */ class DLL_EXPORT ExchangeDelegate diff --git a/src/messaging/ExchangeHolder.h b/src/messaging/ExchangeHolder.h index fb7e2cd39c55bd..379a836f6a5e90 100644 --- a/src/messaging/ExchangeHolder.h +++ b/src/messaging/ExchangeHolder.h @@ -33,12 +33,30 @@ namespace Messaging { * established by ExchangeContext and the transfer of ownership at various points * in its lifetime. * - * It does this by intercepting OnExchangeClosing and looking at the various - * states the exchange might be in to decide how best to correctly shutdown the exchange. - * (see AbortIfNeeded()). + * An equivalent but simplified version of the rules around exchange management as specified in + * ExchangeDelegate.h are provided here for consumers: * - * This is a delegate forwarder - consumers can still register to be an ExchangeDelegate - * and get notified of all relevant happenings on that delegate interface. + * 1. When an exchange is allocated, the holder takes over ownership of the exchange when Grab() is invoked. + * Until a message is sent successfully, the holder will automatically manage the exchange until its + * destructor or Release() is invoked. + * + * 2. If you send a message successfully that doesn't require a response, invoking Get() on the holder there-after will return + * nullptr. + * + * 3. If you send a message successfully that does require a response, invoking Get() on the holder will return a valid + * pointer until the response is received or times out. + * + * 4. On reception of a message on an exchange, if you return from OnMessageReceived() and no messages were sent on that exchange, + * invoking Get() on the holder will return a nullptr. + * + * 5. If you invoke WillSendMessage() on the exchange in your implementation of OnMessageReceived indicating a desire to send a + * message later on the exchange, invoking Get() on the holder will return a valid exchange until SendMessage() on the exchange + * is called, at which point, rules 2 and 3 apply. + * + * 6. This is a delegate forwarder - consumers can still register to be an ExchangeDelegate + * and get notified of all relevant happenings on that delegate interface. + * + * 7. At no point shall you call Abort/Close/Release/Retain on the exchange tracked by the holder. * */ class ExchangeHolder : public ExchangeDelegate @@ -51,7 +69,13 @@ class ExchangeHolder : public ExchangeDelegate */ ExchangeHolder(ExchangeDelegate & delegate) : mpExchangeDelegate(delegate) {} - virtual ~ExchangeHolder() { Release(); } + virtual ~ExchangeHolder() + { +#if CHIP_EXCHANGE_HOLDER_DETAIL_LOGGING + ChipLogDetail(ExchangeManager, "[%p] ~ExchangeHolder", this); +#endif + Release(); + } bool Contains(const ExchangeContext * exchange) const { return mpExchangeCtx == exchange; } @@ -70,6 +94,10 @@ class ExchangeHolder : public ExchangeDelegate mpExchangeCtx = exchange; mpExchangeCtx->SetDelegate(this); + +#if CHIP_EXCHANGE_HOLDER_DETAIL_LOGGING + ChipLogDetail(ExchangeManager, "[%p] ExchangeHolder::Grab: Acquired EC %p", this, exchange); +#endif } /* @@ -78,6 +106,10 @@ class ExchangeHolder : public ExchangeDelegate */ void Release() { +#if CHIP_EXCHANGE_HOLDER_DETAIL_LOGGING + ChipLogDetail(ExchangeManager, "[%p] ExchangeHolder::Release: mpExchangeCtx = %p", this, mpExchangeCtx); +#endif + if (mpExchangeCtx) { mpExchangeCtx->SetDelegate(nullptr); @@ -96,6 +128,9 @@ class ExchangeHolder : public ExchangeDelegate */ if (mpExchangeCtx->IsResponseExpected() || mpExchangeCtx->IsSendExpected()) { +#if CHIP_EXCHANGE_HOLDER_DETAIL_LOGGING + ChipLogDetail(ExchangeManager, "[%p] ExchangeHolder::Release: Aborting!", this); +#endif mpExchangeCtx->Abort(); } } @@ -123,10 +158,26 @@ class ExchangeHolder : public ExchangeDelegate void OnExchangeClosing(ExchangeContext * ec) override { +#if CHIP_EXCHANGE_HOLDER_DETAIL_LOGGING + ChipLogDetail(ExchangeManager, "[%p] ExchangeHolder::OnExchangeClosing: mpExchangeCtx: %p", this, mpExchangeCtx); +#endif + if (mpExchangeCtx) { mpExchangeCtx->SetDelegate(nullptr); - mpExchangeCtx = nullptr; + + /** + * Unless our consumer has signalled an intention to send a message in the future, the exchange + * is owned by the exchange layer and it will automatically handle releasing the ref. So, just null + * out our reference to it. + */ + if (!mpExchangeCtx->IsSendExpected()) + { +#if CHIP_EXCHANGE_HOLDER_DETAIL_LOGGING + ChipLogDetail(ExchangeManager, "[%p] ExchangeHolder::OnExchangeClosing: nulling out ref...", this); +#endif + mpExchangeCtx = nullptr; + } } mpExchangeDelegate.OnExchangeClosing(ec); diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 410183e28e30be..9859854c5d54c0 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -107,7 +107,7 @@ void ExchangeManager::Shutdown() mState = State::kState_NotInitialized; } -ExchangeContext * ExchangeManager::NewContext(const SessionHandle & session, ExchangeDelegate * delegate) +ExchangeContext * ExchangeManager::NewContext(const SessionHandle & session, ExchangeDelegate * delegate, bool isInitiator) { if (!session->IsActiveSession()) { @@ -115,7 +115,7 @@ ExchangeContext * ExchangeManager::NewContext(const SessionHandle & session, Exc ChipLogError(ExchangeManager, "NewContext failed: session inactive"); return nullptr; } - return mContextPool.CreateObject(this, mNextExchangeId++, session, true, delegate); + return mContextPool.CreateObject(this, mNextExchangeId++, session, isInitiator, delegate); } CHIP_ERROR ExchangeManager::RegisterUnsolicitedMessageHandlerForProtocol(Protocols::Id protocolId, diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index e2ad4e8f20e388..c21c0d040de4c5 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -87,16 +87,18 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate /** * Creates a new ExchangeContext with a given peer CHIP node specified by the peer node identifier. * - * @param[in] session The identifier of the secure session (possibly - * the empty session for a non-secure exchange) - * for which the ExchangeContext is being set up. + * @param[in] session The identifier of the secure session (possibly + * the empty session for a non-secure exchange) + * for which the ExchangeContext is being set up. * - * @param[in] delegate A pointer to ExchangeDelegate. + * @param[in] delegate A pointer to ExchangeDelegate. + * @param[in] isInitiator Set to true if the exchange is created on the initiator. This is generally true + * except in unit tests. * * @return A pointer to the created ExchangeContext object On success. Otherwise NULL if no object * can be allocated or is available. */ - ExchangeContext * NewContext(const SessionHandle & session, ExchangeDelegate * delegate); + ExchangeContext * NewContext(const SessionHandle & session, ExchangeDelegate * delegate, bool isInitiator = true); void ReleaseContext(ExchangeContext * ec) { mContextPool.ReleaseObject(ec); } diff --git a/src/messaging/tests/BUILD.gn b/src/messaging/tests/BUILD.gn index 1cb910b415b1e9..4f2fa28bc25825 100644 --- a/src/messaging/tests/BUILD.gn +++ b/src/messaging/tests/BUILD.gn @@ -50,10 +50,14 @@ chip_test_suite("tests") { # And TestAbortExchangesForFabric does not link on EFR32 for some reason. test_sources += [ "TestAbortExchangesForFabric.cpp", - "TestExchangeHolder.cpp", "TestExchangeMgr.cpp", "TestReliableMessageProtocol.cpp", ] + + if (chip_device_platform != "esp32" && chip_device_platform != "mbed" && + chip_device_platform != "nrfconnect") { + test_sources += [ "TestExchangeHolder.cpp" ] + } } cflags = [ "-Wconversion" ] diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp index 24c6ff216d4ae3..6650b6c42f42a5 100644 --- a/src/messaging/tests/MessagingContext.cpp +++ b/src/messaging/tests/MessagingContext.cpp @@ -227,14 +227,14 @@ Messaging::ExchangeContext * MessagingContext::NewUnauthenticatedExchangeToBob(M delegate); } -Messaging::ExchangeContext * MessagingContext::NewExchangeToAlice(Messaging::ExchangeDelegate * delegate) +Messaging::ExchangeContext * MessagingContext::NewExchangeToAlice(Messaging::ExchangeDelegate * delegate, bool isInitiator) { - return mExchangeManager.NewContext(GetSessionBobToAlice(), delegate); + return mExchangeManager.NewContext(GetSessionBobToAlice(), delegate, isInitiator); } -Messaging::ExchangeContext * MessagingContext::NewExchangeToBob(Messaging::ExchangeDelegate * delegate) +Messaging::ExchangeContext * MessagingContext::NewExchangeToBob(Messaging::ExchangeDelegate * delegate, bool isInitiator) { - return mExchangeManager.NewContext(GetSessionAliceToBob(), delegate); + return mExchangeManager.NewContext(GetSessionAliceToBob(), delegate, isInitiator); } void MessageCapturer::OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h index d60f057463806d..5c885300fddac2 100644 --- a/src/messaging/tests/MessagingContext.h +++ b/src/messaging/tests/MessagingContext.h @@ -157,8 +157,8 @@ class MessagingContext : public PlatformMemoryUser Messaging::ExchangeContext * NewUnauthenticatedExchangeToAlice(Messaging::ExchangeDelegate * delegate); Messaging::ExchangeContext * NewUnauthenticatedExchangeToBob(Messaging::ExchangeDelegate * delegate); - Messaging::ExchangeContext * NewExchangeToAlice(Messaging::ExchangeDelegate * delegate); - Messaging::ExchangeContext * NewExchangeToBob(Messaging::ExchangeDelegate * delegate); + Messaging::ExchangeContext * NewExchangeToAlice(Messaging::ExchangeDelegate * delegate, bool isInitiator = true); + Messaging::ExchangeContext * NewExchangeToBob(Messaging::ExchangeDelegate * delegate, bool isInitiator = true); System::Layer & GetSystemLayer() { return mIOContext->GetSystemLayer(); } diff --git a/src/messaging/tests/TestExchangeHolder.cpp b/src/messaging/tests/TestExchangeHolder.cpp index ac7c826ad9d0f5..611e2039b886b5 100644 --- a/src/messaging/tests/TestExchangeHolder.cpp +++ b/src/messaging/tests/TestExchangeHolder.cpp @@ -22,6 +22,7 @@ */ #include "messaging/ExchangeDelegate.h" +#include "system/SystemClock.h" #include #include #include @@ -75,21 +76,36 @@ TestContext * gCtx = nullptr; class MockProtocolResponder : public ExchangeDelegate, public Messaging::UnsolicitedMessageHandler { public: - enum class BehaviorModifier + enum class BehaviorModifier : uint8_t { - kNone, - kHoldMsg1, + kNone = 0x00, + kHoldMsg2 = 0x01, + kErrMsg2 = 0x02, + kExpireSessionBeforeMsg2Send = 0x04, + kExpireSessionAfterMsg2Send = 0x08, + kExpireSessionAfterMsg3Receive = 0x10, }; + template + MockProtocolResponder(BehaviorModifier modifier1, Args &&... args) : + mExchangeCtx(*this), mBehaviorModifier(modifier1, std::forward(args)...) + { + VerifyOrDie(gCtx != nullptr); + gCtx->GetExchangeManager().RegisterUnsolicitedMessageHandlerForProtocol(chip::Protocols::MockProtocol::Id, this); + ChipLogDetail(ExchangeManager, "[%p] MockProtocolResponder: %p", this, &mExchangeCtx); + } + MockProtocolResponder(BehaviorModifier modifier = BehaviorModifier::kNone) : mExchangeCtx(*this) { VerifyOrDie(gCtx != nullptr); - mBehaviorModifier = modifier; + mBehaviorModifier.Set(modifier); gCtx->GetExchangeManager().RegisterUnsolicitedMessageHandlerForProtocol(chip::Protocols::MockProtocol::Id, this); + ChipLogDetail(ExchangeManager, "[%p] MockProtocolResponder: %p", this, &mExchangeCtx); } ~MockProtocolResponder() { + ChipLogDetail(ExchangeManager, "[%p] ~MockProtocolResponder", this); gCtx->GetExchangeManager().UnregisterUnsolicitedMessageHandlerForProtocol(chip::Protocols::MockProtocol::Id); } @@ -108,24 +124,41 @@ class MockProtocolResponder : public ExchangeDelegate, public Messaging::Unsolic void OnResponseTimeout(ExchangeContext * ec) override {} ExchangeHolder mExchangeCtx; - BehaviorModifier mBehaviorModifier = BehaviorModifier::kNone; - bool mInteractionSucceeded = false; + BitFlags mBehaviorModifier = BehaviorModifier::kNone; + bool mInteractionSucceeded = false; }; class MockProtocolInitiator : public ExchangeDelegate { public: - enum class BehaviorModifier + enum class BehaviorModifier : uint8_t { - kNone, - kHoldMsg2, + kNone = 0x00, + kHoldMsg3 = 0x01, + kErrMsg1 = 0x02, + kErrMsg3 = 0x04, + kDontSendMsg1 = 0x08, + kExpireSessionBeforeMsg1Send = 0x10, + kExpireSessionAfterMsg1Send = 0x12, + kExpireSessionBeforeMsg3Send = 0x14, + kExpireSessionAfterMsg3Send = 0x18, }; MockProtocolInitiator(BehaviorModifier modifier = BehaviorModifier::kNone) : mExchangeCtx(*this) { - mBehaviorModifier = modifier; + mBehaviorModifier.Set(modifier); + ChipLogDetail(ExchangeManager, "[%p] MockProtocolInitiator: %p", this, &mExchangeCtx); + } + + template + MockProtocolInitiator(BehaviorModifier modifier1, Args &&... args) : + mExchangeCtx(*this), mBehaviorModifier(modifier1, std::forward(args)...) + { + ChipLogDetail(ExchangeManager, "[%p] MockProtocolInitiator: %p", this, &mExchangeCtx); } + ~MockProtocolInitiator() { ChipLogDetail(ExchangeManager, "[%p] ~MockProtocolInitiator", this); } + CHIP_ERROR StartInteraction(SessionHandle & sessionHandle); bool DidInteractionSucceed() { return mInteractionSucceeded; } @@ -137,8 +170,8 @@ class MockProtocolInitiator : public ExchangeDelegate void OnResponseTimeout(ExchangeContext * ec) override {} ExchangeHolder mExchangeCtx; - BehaviorModifier mBehaviorModifier = BehaviorModifier::kNone; - bool mInteractionSucceeded = false; + BitFlags mBehaviorModifier = BehaviorModifier::kNone; + bool mInteractionSucceeded = false; }; CHIP_ERROR MockProtocolResponder::OnMessageReceived(ExchangeContext * ec, const PayloadHeader & payloadHeader, @@ -153,12 +186,39 @@ CHIP_ERROR MockProtocolResponder::OnMessageReceived(ExchangeContext * ec, const // mExchangeCtx.Grab(ec); - if (mBehaviorModifier != BehaviorModifier::kHoldMsg1) + if (!mBehaviorModifier.Has(BehaviorModifier::kHoldMsg2)) { PacketBufferHandle respBuffer = MessagePacketBuffer::New(0); VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_NO_MEMORY); - ReturnErrorOnFailure(mExchangeCtx->SendMessage(chip::Protocols::MockProtocol::MessageType::kMsg2, std::move(respBuffer), - SendMessageFlags::kExpectResponse)); + + if (mBehaviorModifier.Has(BehaviorModifier::kErrMsg2)) + { + mExchangeCtx->InjectFailure(ExchangeContext::InjectedFailureType::kFailOnSend); + } + + if (mBehaviorModifier.Has(BehaviorModifier::kExpireSessionBeforeMsg2Send)) + { + mExchangeCtx->GetSessionHolder().Release(); + mExchangeCtx->OnSessionReleased(); + } + + if (mExchangeCtx) + { + err = mExchangeCtx->SendMessage(chip::Protocols::MockProtocol::MessageType::kMsg2, std::move(respBuffer), + SendMessageFlags::kExpectResponse); + if (mExchangeCtx) + { + mExchangeCtx->ClearInjectedFailures(); + } + + ReturnErrorOnFailure(err); + } + + if (mBehaviorModifier.Has(BehaviorModifier::kExpireSessionAfterMsg2Send)) + { + mExchangeCtx->GetSessionHolder().Release(); + mExchangeCtx->OnSessionReleased(); + } } else { @@ -167,6 +227,12 @@ CHIP_ERROR MockProtocolResponder::OnMessageReceived(ExchangeContext * ec, const } else if (payloadHeader.HasMessageType(chip::Protocols::MockProtocol::MessageType::kMsg3)) { + if (mBehaviorModifier.Has(BehaviorModifier::kExpireSessionAfterMsg3Receive)) + { + mExchangeCtx->GetSessionHolder().Release(); + mExchangeCtx->OnSessionReleased(); + } + mInteractionSucceeded = true; } else @@ -190,8 +256,34 @@ CHIP_ERROR MockProtocolInitiator::StartInteraction(SessionHandle & sessionHandle // mExchangeCtx.Grab(exchange); - ReturnErrorOnFailure(mExchangeCtx->SendMessage(chip::Protocols::MockProtocol::MessageType::kMsg1, std::move(buffer), - SendMessageFlags::kExpectResponse)); + if (mBehaviorModifier.Has(BehaviorModifier::kErrMsg1)) + { + mExchangeCtx->InjectFailure(ExchangeContext::InjectedFailureType::kFailOnSend); + } + + if (mBehaviorModifier.Has(BehaviorModifier::kExpireSessionBeforeMsg1Send)) + { + mExchangeCtx->GetSessionHolder().Release(); + mExchangeCtx->OnSessionReleased(); + } + + if (!mBehaviorModifier.Has(BehaviorModifier::kDontSendMsg1)) + { + auto err = mExchangeCtx->SendMessage(chip::Protocols::MockProtocol::MessageType::kMsg1, std::move(buffer), + SendMessageFlags::kExpectResponse); + if (mExchangeCtx) + { + mExchangeCtx->ClearInjectedFailures(); + } + + ReturnErrorOnFailure(err); + } + + if (mBehaviorModifier.Has(BehaviorModifier::kExpireSessionAfterMsg1Send)) + { + mExchangeCtx->GetSessionHolder().Release(); + mExchangeCtx->OnSessionReleased(); + } return CHIP_NO_ERROR; } @@ -203,12 +295,39 @@ CHIP_ERROR MockProtocolInitiator::OnMessageReceived(ExchangeContext * ec, const if (payloadHeader.HasMessageType(chip::Protocols::MockProtocol::MessageType::kMsg2)) { - if (mBehaviorModifier != BehaviorModifier::kHoldMsg2) + if (!mBehaviorModifier.Has(BehaviorModifier::kHoldMsg3)) { + if (mBehaviorModifier.Has(BehaviorModifier::kExpireSessionBeforeMsg3Send)) + { + mExchangeCtx->GetSessionHolder().Release(); + mExchangeCtx->OnSessionReleased(); + } + PacketBufferHandle respBuffer = MessagePacketBuffer::New(0); VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_NO_MEMORY); - ReturnErrorOnFailure(mExchangeCtx->SendMessage(chip::Protocols::MockProtocol::MessageType::kMsg3, std::move(respBuffer), - SendMessageFlags::kNone)); + + if (mBehaviorModifier.Has(BehaviorModifier::kErrMsg3)) + { + mExchangeCtx->InjectFailure(ExchangeContext::InjectedFailureType::kFailOnSend); + } + + if (mExchangeCtx) + { + err = mExchangeCtx->SendMessage(chip::Protocols::MockProtocol::MessageType::kMsg3, std::move(respBuffer), + SendMessageFlags::kNone); + if (mExchangeCtx) + { + mExchangeCtx->ClearInjectedFailures(); + } + + ReturnErrorOnFailure(err); + } + + if (mBehaviorModifier.Has(BehaviorModifier::kExpireSessionAfterMsg3Send)) + { + mExchangeCtx->GetSessionHolder().Release(); + mExchangeCtx->OnSessionReleased(); + } mInteractionSucceeded = true; } @@ -233,8 +352,117 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) auto sessionHandle = ctx.GetSessionAliceToBob(); + ctx.SetMRPMode(chip::Test::MessagingContext::MRPMode::kResponsive); + + // + // #1: Initiator (AllocExchange) + // + // The initiator just allocated the exchange, but doesn't send a message on it. + // + // Then, destroy both objects. Initiator's holder should correctly abort the exchange since it still owns + // it. + // + { + ChipLogProgress(ExchangeManager, "-------- #1: Initiator (AllocExchange) ----------"); + + { + MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kDontSendMsg1); + MockProtocolResponder responder; + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + } + + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #2: Initiator --X Msg1 + // + // Inject a failure to transmit Msg1. This should retain the WillSendMessage flag on the initiator's exchange. + // + // Then, destroy both objects. Initiator's holder should correctly abort the exchange since it's still has the + // WillSendMessage flag on it. + // + // + { + ChipLogProgress(ExchangeManager, "-------- #2: Initiator --X (SendErr) Msg1 --------- "); + + { + MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kErrMsg1); + MockProtocolResponder responder; + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR); + } + + // + // Service IO AFTER the objects above cease to exist to prevent Msg1 from getting to Responder. This also + // flush any pending messages in the queue. + // + ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #3: Initiator --X (Session Released Before) -- Msg1 + // + // Inject a release of the session associated with the exchange on the initiator before sending Msg1. This + // should just close out the exchange without releasing the ref. // - // #1: Initiator >--- Msg1 --X Responder. + // Then, destroy both objects. The initiator's holder should correctly abort the exchange since WillSendMessage + // should still be present on the EC. + // + { + ChipLogProgress(ExchangeManager, "-------- #3: Initiator --X (SessionReleased before) Msg1 --------- "); + + { + MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kExpireSessionBeforeMsg1Send); + MockProtocolResponder responder; + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR); + } + + // + // Service IO AFTER the objects above cease to exist to prevent Msg1 from getting to Responder. This also + // flush any pending messages in the queue. + // + ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #4: Initiator --X (SendErr + Session Released After) -- Msg1 + // + // Inject an error at Msg1 transmission followed by the release of a session (scenario in #21544). This should + // just close out the exchange without releasing the ref since the WillSendMessage flag should still be set. + // + // Then, destroy both objects. The initiator's holder should correctly abort the exchange since WillSendMessage + // should still be present on the EC. + // + { + ChipLogProgress(ExchangeManager, "-------- #4: Initiator --X (SendErr + SessionReleased after) Msg1 --------- "); + + { + MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kExpireSessionAfterMsg1Send, + MockProtocolInitiator::BehaviorModifier::kErrMsg1); + MockProtocolResponder responder; + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR); + } + + // + // Service IO AFTER the objects above cease to exist to prevent Msg1 from getting to Responder. This also + // flush any pending messages in the queue. + // + ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #5: Initiator >--- Msg1 --X Responder. // // Initiator sends Msg1 to Responder, but we set it up such that Responder doesn't actually // receive the message. @@ -243,7 +471,7 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) // a response. // { - ChipLogProgress(ExchangeManager, "-------- #1: Initiator >-- Msg1 --X Responder ---------"); + ChipLogProgress(ExchangeManager, "-------- #5: Initiator >-- Msg1 --X Responder ---------"); { MockProtocolInitiator initiator; @@ -262,7 +490,7 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) } // - // #2: Initiator --- Msg1 --> Responder (WillSend) + // #6: Initiator --- Msg1 --> Responder (WillSend) // // Initiator sends Msg1 to Responder, which is received successfully. However, Responder // doesn't send a response right away (calls WillSendMessage() on the EC). @@ -272,10 +500,10 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) // { { - ChipLogProgress(ExchangeManager, "-------- #2: Initiator >-- Msg1 --> Responder (WillSend) ---------"); + ChipLogProgress(ExchangeManager, "-------- #6: Initiator >-- Msg1 --> Responder (WillSend) ---------"); MockProtocolInitiator initiator; - MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kHoldMsg1); + MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kHoldMsg2); auto err = initiator.StartInteraction(sessionHandle); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -287,8 +515,86 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) } // - // #3: Initiator --- Msg1 --> Responder - // (WillSend) Initiator <-- Msg2 <-- Responder + // #7: Initiator --- Msg1 --> Responder + // Msg2 (SendErr) X-- Responder + // + // Inject an error in the responder when attempting to send Msg2. + // + // Then, destroy both objects. The holder on the responder should abort the exchange since + // the transmission failed, and the ref is still with the holder. The holder on the initiator + // should abort the exchange since it is waiting for a response. + // + // + { + { + ChipLogProgress(ExchangeManager, "-------- #7: Msg2 (SendFailure) X-- Responder ---------"); + + MockProtocolInitiator initiator; + MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kErrMsg2); + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + } + + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #8: Initiator --- Msg1 --> Responder + // Msg2 (SessionReleased before) X-- Responder + // + // Release the session right before sending Msg2 on the responder. This should abort the underlying exchange + // immediately since neither WillSendMessage or ResponseExpected flags are set. + // + // Then, destroy both objects. The holders on both should just null out their internal reference to the EC. + // + { + { + ChipLogProgress(ExchangeManager, "-------- #8: Msg2 (SessionReleased Before) X-- Responder ---------"); + + MockProtocolInitiator initiator; + MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kExpireSessionBeforeMsg2Send); + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + } + + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #9: Initiator --- Msg1 --> Responder + // Msg2 (SendErr + SessionReleased after) X-- Responder + // + // Trigger a send error when sending Msg2 from the responder, and release the session immediately after. This should still + // preserve the WillSendMessage flags on the exchange and just close out the EC without releasing the ref. + // + // Then, destroy both objects. The holders on both should abort their respective ECs. + // + { + { + ChipLogProgress(ExchangeManager, "-------- #9: Msg2 (SendErr + SessionReleased after) X-- Responder ---------"); + + MockProtocolInitiator initiator; + MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kErrMsg2, + MockProtocolResponder::BehaviorModifier::kExpireSessionAfterMsg2Send); + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + } + + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #10: Initiator --- Msg1 --> Responder + // (WillSend) Initiator <-- Msg2 <-- Responder // // Initiator receives Msg2 back from Responder, but calls WillSend on that EC. // @@ -297,9 +603,9 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) // { { - ChipLogProgress(ExchangeManager, "-------- #3: (WillSend) Initiator <-- Msg2 <-- Responder ---------"); + ChipLogProgress(ExchangeManager, "-------- #10: (WillSend) Initiator <-- Msg2 <-- Responder ---------"); - MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kHoldMsg2); + MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kHoldMsg3); MockProtocolResponder responder; auto err = initiator.StartInteraction(sessionHandle); @@ -312,9 +618,62 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) } // - // #4: Initiator --- Msg1 --> Responder - // Initiator <-- Msg2 <-- Responder - // Initiator >-- Msg3 --> Responder + // #11: Initiator --- Msg1 --> Responder + // Initiator <-- Msg2 <-- Responder + // Initiator (SessionReleased before) X-- Msg3 + // + // Release the session right before the initiator sends Msg3. This should abort the underlying EC immediately on the initiator. + // + // Then destroy both objects. Both holders on the initiator and responder should be pointing to null. + // + { + { + ChipLogProgress(ExchangeManager, "-------- #11: Initiator --X (SessionReleased before) Msg3 ------------"); + + MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kExpireSessionBeforeMsg3Send); + MockProtocolResponder responder; + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + } + + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #12: Initiator --- Msg1 --> Responder + // Initiator <-- Msg2 <-- Responder + // Initiator X (SendErr + SessionReleased after) -- Msg3 + // + // Trigger a send error on the initiator when sending Msg3, followed by a session release. Since a send was initiated, the ref + // is with the initiator's holder and the EC will just close itself out without removing the ref. + // + // Then, destroy both objects. The responder's holder will have a null ref but the initiator's holder will have a non-null ref, + // and should abort it. + // + { + { + ChipLogProgress(ExchangeManager, "-------- #12: Initiator --X (SendErr + SessionReleased after) Msg3 ------------"); + + MockProtocolInitiator initiator(MockProtocolInitiator::BehaviorModifier::kErrMsg3, + MockProtocolInitiator::BehaviorModifier::kExpireSessionAfterMsg3Send); + MockProtocolResponder responder; + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + } + + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #13: Initiator --- Msg1 --> Responder + // Initiator <-- Msg2 <-- Responder + // Initiator >-- Msg3 --> Responder // // Initiator sends final message in exchange to Responder, which is received successfully. // @@ -324,7 +683,7 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) // { { - ChipLogProgress(ExchangeManager, "-------- #4: Initiator >-- Msg3 --> Responder ---------"); + ChipLogProgress(ExchangeManager, "-------- #13: Initiator >-- Msg3 --> Responder ---------"); MockProtocolInitiator initiator; MockProtocolResponder responder; @@ -339,20 +698,60 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) } // - // #5: Initiator --- Msg1 --> Responder (WillSend) + // #14: Initiator --- Msg1 --> Responder + // Initiator <-- Msg2 <-- Responder + // Initiator >-- Msg3 --> Responder (SessionReleased) + // + // Released the session right on reception of Msg3 on the responder. Since there no responses expected or send expected, + // the EC aborts immediately. + // + // Then, destroy both objects. Both holders should be point to null and should do nothing. + // + { + { + ChipLogProgress(ExchangeManager, "-------- #14: Initiator >-- Msg3 --> Responder (SessionReleased) ---------"); + + MockProtocolInitiator initiator; + MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kExpireSessionAfterMsg3Receive); + + auto err = initiator.StartInteraction(sessionHandle); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + + // + // Because of the session expiration right after Msg3 is received, it causes an abort of the underlying EC + // on the reponder side. This means that Msg3 won't be ACK'ed. Msg3 on the initiator side remains un-acked. Since + // the exchange was just closed and not aborted on the initiator side, it is still sitting in the retransmission table + // and consequently, the exchange still has a ref-count of 1. If we were to just check the number of active + // exchanges, it would still show 1 active exchange. + // + // This will only be released once the re-transmission table + // entry has been removed. To make this happen, drive the IO forward enough that a single re-transmission happens. This + // will result in a duplicate message ACK being delivered by the responder, causing the EC to finally get released. + // + ctx.GetIOContext().DriveIOUntil(System::Clock::Seconds16(5), + [&]() { return ctx.GetExchangeManager().GetNumActiveExchanges() == 0; }); + } + + NL_TEST_ASSERT(inSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } + + // + // #15: Initiator --- Msg1 --> Responder (WillSend) // Initiator --- Msg1 --> Responder (WillSend) // - // Similar to #2, except we have Initiator start the interaction again. This validates + // Similar to #6, except we have Initiator start the interaction again. This validates // ExchangeHolder::Grab in correctly aborting a previous exchange and acquiring a new one. // - // Then, destroy both objects. Both holders should abort the exchange (see #2). + // Then, destroy both objects. Both holders should abort the exchange (see #6). // { { - ChipLogProgress(ExchangeManager, "-------- #5: Initiator >-- Msg1 --> Responder (WillSend) X2 ---------"); + ChipLogProgress(ExchangeManager, "-------- #15: Initiator >-- Msg1 --> Responder (WillSend) X2 ---------"); MockProtocolInitiator initiator; - MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kHoldMsg1); + MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kHoldMsg2); auto err = initiator.StartInteraction(sessionHandle); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -370,21 +769,20 @@ void TestExchangeHolder(nlTestSuite * inSuite, void * inContext) } // - // #6: Initiator --- Msg1 --> Responder + // #16: Initiator --- Msg1 --> Responder // Initiator <-- Msg2 <-- Responder // Initiator >-- Msg3 --> Responder // // X2 // - // Similar to #4, except we do the entire interaction twice. This validates - // ExchangeHolder::Grab in correctly releasing a reference to a previous exchange (but not aborting it) - // and acquiring a new one. + // We do the entire interaction twice. This validates ExchangeHolder::Grab in correctly releasing a reference + // to a previous exchange (but not aborting it) and acquiring a new one. // // Then, destroy both objects. Both holders should release their reference without aborting. // { { - ChipLogProgress(ExchangeManager, "-------- #6: Initiator >-- Msg3 --> Responder X2 ---------"); + ChipLogProgress(ExchangeManager, "-------- #16: Initiator >-- Msg3 --> Responder X2 ---------"); MockProtocolInitiator initiator; MockProtocolResponder responder; diff --git a/src/transport/tests/LoopbackTransportManager.h b/src/transport/tests/LoopbackTransportManager.h index 1ee6f8c7d1372d..92703754a1735c 100644 --- a/src/transport/tests/LoopbackTransportManager.h +++ b/src/transport/tests/LoopbackTransportManager.h @@ -81,6 +81,7 @@ class LoopbackTransportManager return; } } + // Processing those messages might have queued some run-ASAP async // work. Make sure to process that too, in case it generates // response messages.