Fix ExchangeContext leaks (#21846)
diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp
index 7bd0aaf..9d3c8ec 100644
--- a/src/messaging/ExchangeContext.cpp
+++ b/src/messaging/ExchangeContext.cpp
@@ -139,13 +139,6 @@
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 @@
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 @@
}
// 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 @@
void ExchangeContext::DoClose(bool clearRetransTable)
{
+ if (mFlags.Has(Flags::kFlagClosed))
+ {
+ return;
+ }
+
mFlags.Set(Flags::kFlagClosed);
// Clear protocol callbacks
@@ -336,7 +351,11 @@
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 @@
// 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 3e7e561..4631cb0 100644
--- a/src/messaging/ExchangeContext.h
+++ b/src/messaging/ExchangeContext.h
@@ -212,7 +212,24 @@
*/
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<InjectedFailureType> mInjectedFailures;
+#endif
+
class ExchangeSessionHolder : public SessionHolderWithDelegate
{
public:
diff --git a/src/messaging/ExchangeDelegate.h b/src/messaging/ExchangeDelegate.h
index 363f050..97a5a0a 100644
--- a/src/messaging/ExchangeDelegate.h
+++ b/src/messaging/ExchangeDelegate.h
@@ -61,13 +61,13 @@
* 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 fb7e2cd..379a836 100644
--- a/src/messaging/ExchangeHolder.h
+++ b/src/messaging/ExchangeHolder.h
@@ -33,12 +33,30 @@
* 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 @@
*/
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 @@
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 @@
*/
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 @@
*/
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 @@
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 410183e..9859854 100644
--- a/src/messaging/ExchangeMgr.cpp
+++ b/src/messaging/ExchangeMgr.cpp
@@ -107,7 +107,7 @@
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 @@
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 e2ad4e8..c21c0d0 100644
--- a/src/messaging/ExchangeMgr.h
+++ b/src/messaging/ExchangeMgr.h
@@ -87,16 +87,18 @@
/**
* 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 1cb910b..4f2fa28 100644
--- a/src/messaging/tests/BUILD.gn
+++ b/src/messaging/tests/BUILD.gn
@@ -50,10 +50,14 @@
# 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 24c6ff2..6650b6c 100644
--- a/src/messaging/tests/MessagingContext.cpp
+++ b/src/messaging/tests/MessagingContext.cpp
@@ -227,14 +227,14 @@
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 d60f057..5c88530 100644
--- a/src/messaging/tests/MessagingContext.h
+++ b/src/messaging/tests/MessagingContext.h
@@ -157,8 +157,8 @@
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 ac7c826..611e203 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 <lib/support/UnitTestContext.h>
#include <lib/support/UnitTestRegistration.h>
#include <lib/support/UnitTestUtils.h>
@@ -75,21 +76,36 @@
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 <typename... Args>
+ MockProtocolResponder(BehaviorModifier modifier1, Args &&... args) :
+ mExchangeCtx(*this), mBehaviorModifier(modifier1, std::forward<Args>(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 @@
void OnResponseTimeout(ExchangeContext * ec) override {}
ExchangeHolder mExchangeCtx;
- BehaviorModifier mBehaviorModifier = BehaviorModifier::kNone;
- bool mInteractionSucceeded = false;
+ BitFlags<BehaviorModifier> 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 <typename... Args>
+ MockProtocolInitiator(BehaviorModifier modifier1, Args &&... args) :
+ mExchangeCtx(*this), mBehaviorModifier(modifier1, std::forward<Args>(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 @@
void OnResponseTimeout(ExchangeContext * ec) override {}
ExchangeHolder mExchangeCtx;
- BehaviorModifier mBehaviorModifier = BehaviorModifier::kNone;
- bool mInteractionSucceeded = false;
+ BitFlags<BehaviorModifier> mBehaviorModifier = BehaviorModifier::kNone;
+ bool mInteractionSucceeded = false;
};
CHIP_ERROR MockProtocolResponder::OnMessageReceived(ExchangeContext * ec, const PayloadHeader & payloadHeader,
@@ -153,12 +186,39 @@
//
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 @@
}
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 @@
//
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 @@
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 @@
auto sessionHandle = ctx.GetSessionAliceToBob();
+ ctx.SetMRPMode(chip::Test::MessagingContext::MRPMode::kResponsive);
+
//
- // #1: Initiator >--- Msg1 --X Responder.
+ // #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.
+ //
+ // 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 @@
// a response.
//
{
- ChipLogProgress(ExchangeManager, "-------- #1: Initiator >-- Msg1 --X Responder ---------");
+ ChipLogProgress(ExchangeManager, "-------- #5: Initiator >-- Msg1 --X Responder ---------");
{
MockProtocolInitiator initiator;
@@ -262,7 +490,7 @@
}
//
- // #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 @@
//
{
{
- 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 @@
}
//
- // #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 @@
//
{
{
- 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 @@
}
//
- // #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 @@
//
{
{
- ChipLogProgress(ExchangeManager, "-------- #4: Initiator >-- Msg3 --> Responder ---------");
+ ChipLogProgress(ExchangeManager, "-------- #13: Initiator >-- Msg3 --> Responder ---------");
MockProtocolInitiator initiator;
MockProtocolResponder responder;
@@ -339,20 +698,60 @@
}
//
- // #5: Initiator --- Msg1 --> Responder (WillSend)
- // Initiator --- Msg1 --> Responder (WillSend)
+ // #14: Initiator --- Msg1 --> Responder
+ // Initiator <-- Msg2 <-- Responder
+ // Initiator >-- Msg3 --> Responder (SessionReleased)
//
- // Similar to #2, except we have Initiator start the interaction again. This validates
- // ExchangeHolder::Grab in correctly aborting a previous exchange and acquiring a new one.
+ // 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 abort the exchange (see #2).
+ // Then, destroy both objects. Both holders should be point to null and should do nothing.
//
{
{
- ChipLogProgress(ExchangeManager, "-------- #5: Initiator >-- Msg1 --> Responder (WillSend) X2 ---------");
+ ChipLogProgress(ExchangeManager, "-------- #14: Initiator >-- Msg3 --> Responder (SessionReleased) ---------");
MockProtocolInitiator initiator;
- MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kHoldMsg1);
+ 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 #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 #6).
+ //
+ {
+ {
+ ChipLogProgress(ExchangeManager, "-------- #15: Initiator >-- Msg1 --> Responder (WillSend) X2 ---------");
+
+ MockProtocolInitiator initiator;
+ MockProtocolResponder responder(MockProtocolResponder::BehaviorModifier::kHoldMsg2);
auto err = initiator.StartInteraction(sessionHandle);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
@@ -370,21 +769,20 @@
}
//
- // #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;
