Skip to content

Commit

Permalink
Add unit test for CRMP retransmit with closed exchange (#7054)
Browse files Browse the repository at this point in the history 
* Add unit test for CRMP retransmit with closed exchange

* Update src/messaging/tests/TestReliableMessageProtocol.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* address review comments

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
  • Loading branch information
@pan-apple @bzbarsky-apple @pull
2 people authored and pull[bot] committed Aug 5, 2021
1 parent a66c69b commit 7ce0fe3
Show file tree
Hide file tree
Showing 13 changed files with 147 additions and 88 deletions.
60 changes: 27 additions & 33 deletions src/messaging/ExchangeContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,6 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
reliableTransmissionRequested = !sendFlags.Has(SendMessageFlags::kNoAutoRequestAck);
}

ExchangeMessageDispatch * dispatch = GetMessageDispatch();
ApplicationExchangeDispatch defaultDispatch;

if (dispatch == nullptr)
{
defaultDispatch.Init(mExchangeMgr->GetReliableMessageMgr(), mExchangeMgr->GetSessionMgr());
dispatch = &defaultDispatch;
}

// If a response message is expected...
if (sendFlags.Has(SendMessageFlags::kExpectResponse))
{
Expand All @@ -130,8 +121,8 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
}
}

err = dispatch->SendMessage(mSecureSession, mExchangeId, IsInitiator(), GetReliableMessageContext(),
reliableTransmissionRequested, protocolId, msgType, std::move(msgBuf));
err = mDispatch->SendMessage(mSecureSession, mExchangeId, IsInitiator(), GetReliableMessageContext(),
reliableTransmissionRequested, protocolId, msgType, std::move(msgBuf));

exit:
if (err != CHIP_NO_ERROR && IsResponseExpected())
Expand Down Expand Up @@ -217,18 +208,8 @@ void ExchangeContextDeletor::Release(ExchangeContext * ec)
ec->mExchangeMgr->ReleaseContext(ec);
}

ExchangeMessageDispatch * ExchangeContext::GetMessageDispatch()
{
if (mDelegate != nullptr)
{
return mDelegate->GetMessageDispatch(mExchangeMgr->GetReliableMessageMgr(), mExchangeMgr->GetSessionMgr());
}

return nullptr;
}

ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, SecureSessionHandle session, bool Initiator,
ExchangeDelegateBase * delegate)
ExchangeDelegate * delegate)
{
VerifyOrDie(mExchangeMgr == nullptr);

Expand All @@ -238,6 +219,22 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, Secu
mFlags.Set(Flags::kFlagInitiator, Initiator);
mDelegate = delegate;

ExchangeMessageDispatch * dispatch = nullptr;
if (delegate != nullptr)
{
dispatch = delegate->GetMessageDispatch(em->GetReliableMessageMgr(), em->GetSessionMgr());
if (dispatch == nullptr)
{
dispatch = &em->mDefaultExchangeDispatch;
}
}
else
{
dispatch = &em->mDefaultExchangeDispatch;
}
VerifyOrDie(dispatch != nullptr);
mDispatch = dispatch->Retain();

SetDropAckDebug(false);
SetAckPending(false);
SetPeerRequestedAck(false);
Expand Down Expand Up @@ -267,6 +264,12 @@ ExchangeContext::~ExchangeContext()
mExchangeACL = nullptr;
}

if (mDispatch != nullptr)
{
mDispatch->Release();
mDispatch = nullptr;
}

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
ChipLogDetail(ExchangeManager, "ec-- id: %d", mExchangeId);
#endif
Expand Down Expand Up @@ -331,7 +334,7 @@ void ExchangeContext::HandleResponseTimeout(System::Layer * aSystemLayer, void *
// NOTE: we don't set mResponseExpected to false here because the response could still arrive. If the user
// wants to never receive the response, they must close the exchange context.

ExchangeDelegateBase * delegate = ec->GetDelegate();
ExchangeDelegate * delegate = ec->GetDelegate();

// Call the user's timeout handler.
if (delegate != nullptr)
Expand All @@ -347,17 +350,8 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con
// layer has completed its work on the ExchangeContext.
Retain();

ExchangeMessageDispatch * dispatch = GetMessageDispatch();
ApplicationExchangeDispatch defaultDispatch;

if (dispatch == nullptr)
{
defaultDispatch.Init(mExchangeMgr->GetReliableMessageMgr(), mExchangeMgr->GetSessionMgr());
dispatch = &defaultDispatch;
}

CHIP_ERROR err =
dispatch->OnMessageReceived(payloadHeader, packetHeader.GetMessageId(), peerAddress, GetReliableMessageContext());
mDispatch->OnMessageReceived(payloadHeader, packetHeader.GetMessageId(), peerAddress, GetReliableMessageContext());
SuccessOrExit(err);

// The SecureChannel::StandaloneAck message type is only used for CRMP; do not pass such messages to the application layer.
Expand Down
16 changes: 9 additions & 7 deletions src/messaging/ExchangeContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
typedef uint32_t Timeout; // Type used to express the timeout in this ExchangeContext, in milliseconds

ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, SecureSessionHandle session, bool Initiator,
ExchangeDelegateBase * delegate);
ExchangeDelegate * delegate);

~ExchangeContext();

Expand Down Expand Up @@ -128,14 +128,14 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
CHIP_ERROR HandleMessage(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
const Transport::PeerAddress & peerAddress, System::PacketBufferHandle && msgBuf);

ExchangeDelegateBase * GetDelegate() const { return mDelegate; }
void SetDelegate(ExchangeDelegateBase * delegate) { mDelegate = delegate; }
ExchangeDelegate * GetDelegate() const { return mDelegate; }
void SetDelegate(ExchangeDelegate * delegate) { mDelegate = delegate; }

ExchangeManager * GetExchangeMgr() const { return mExchangeMgr; }

ReliableMessageContext * GetReliableMessageContext() { return static_cast<ReliableMessageContext *>(this); };

ExchangeMessageDispatch * GetMessageDispatch();
ExchangeMessageDispatch * GetMessageDispatch() { return mDispatch; }

ExchangeACL * GetExchangeACL(Transport::AdminPairingTable & table)
{
Expand Down Expand Up @@ -167,9 +167,11 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen

private:
Timeout mResponseTimeout; // Maximum time to wait for response (in milliseconds); 0 disables response timeout.
ExchangeDelegateBase * mDelegate = nullptr;
ExchangeManager * mExchangeMgr = nullptr;
ExchangeACL * mExchangeACL = nullptr;
ExchangeDelegate * mDelegate = nullptr;
ExchangeManager * mExchangeMgr = nullptr;
ExchangeACL * mExchangeACL = nullptr;

ExchangeMessageDispatch * mDispatch = nullptr;

SecureSessionHandle mSecureSession; // The connection state
uint16_t mExchangeId; // Assigned exchange ID.
Expand Down
20 changes: 4 additions & 16 deletions src/messaging/ExchangeDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ class ExchangeContext;
* is interested in receiving these callbacks, they can specialize this class and handle
* each trigger in their implementation of this class.
*/
class DLL_EXPORT ExchangeDelegateBase
class DLL_EXPORT ExchangeDelegate
{
public:
virtual ~ExchangeDelegateBase() {}
virtual ~ExchangeDelegate() {}

/**
* @brief
Expand Down Expand Up @@ -77,22 +77,10 @@ class DLL_EXPORT ExchangeDelegateBase
*/
virtual void OnExchangeClosing(ExchangeContext * ec) {}

virtual ExchangeMessageDispatch * GetMessageDispatch(ReliableMessageMgr * rmMgr, SecureSessionMgr * sessionMgr) = 0;
};

class DLL_EXPORT ExchangeDelegate : public ExchangeDelegateBase
{
public:
virtual ~ExchangeDelegate() {}

virtual ExchangeMessageDispatch * GetMessageDispatch(ReliableMessageMgr * rmMgr, SecureSessionMgr * sessionMgr)
virtual ExchangeMessageDispatch * GetMessageDispatch(ReliableMessageMgr * reliableMessageMgr, SecureSessionMgr * sessionMgr)
{
mMessageDispatch.Init(rmMgr, sessionMgr);
return &mMessageDispatch;
return nullptr;
}

private:
ApplicationExchangeDispatch mMessageDispatch;
};

} // namespace Messaging
Expand Down
3 changes: 2 additions & 1 deletion src/messaging/ExchangeMessageDispatch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#pragma once

#include <lib/core/ReferenceCounted.h>
#include <transport/SecureSessionMgr.h>

namespace chip {
Expand All @@ -31,7 +32,7 @@ namespace Messaging {
class ReliableMessageMgr;
class ReliableMessageContext;

class ExchangeMessageDispatch
class ExchangeMessageDispatch : public ReferenceCounted<ExchangeMessageDispatch>
{
public:
ExchangeMessageDispatch() {}
Expand Down
11 changes: 6 additions & 5 deletions src/messaging/ExchangeMgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ CHIP_ERROR ExchangeManager::Init(SecureSessionMgr * sessionMgr)
sessionMgr->SetDelegate(this);

mReliableMessageMgr.Init(sessionMgr->SystemLayer(), sessionMgr);
ReturnErrorOnFailure(mDefaultExchangeDispatch.Init(&mReliableMessageMgr, mSessionMgr));

mState = State::kState_Initialized;

Expand Down Expand Up @@ -113,18 +114,18 @@ CHIP_ERROR ExchangeManager::Shutdown()
return CHIP_NO_ERROR;
}

ExchangeContext * ExchangeManager::NewContext(SecureSessionHandle session, ExchangeDelegateBase * delegate)
ExchangeContext * ExchangeManager::NewContext(SecureSessionHandle session, ExchangeDelegate * delegate)
{
return mContextPool.CreateObject(this, mNextExchangeId++, session, true, delegate);
}

CHIP_ERROR ExchangeManager::RegisterUnsolicitedMessageHandlerForProtocol(Protocols::Id protocolId, ExchangeDelegateBase * delegate)
CHIP_ERROR ExchangeManager::RegisterUnsolicitedMessageHandlerForProtocol(Protocols::Id protocolId, ExchangeDelegate * delegate)
{
return RegisterUMH(protocolId, kAnyMessageType, delegate);
}

CHIP_ERROR ExchangeManager::RegisterUnsolicitedMessageHandlerForType(Protocols::Id protocolId, uint8_t msgType,
ExchangeDelegateBase * delegate)
ExchangeDelegate * delegate)
{
return RegisterUMH(protocolId, static_cast<int16_t>(msgType), delegate);
}
Expand All @@ -144,7 +145,7 @@ void ExchangeManager::OnReceiveError(CHIP_ERROR error, const Transport::PeerAddr
ChipLogError(ExchangeManager, "Accept FAILED, err = %s", ErrorStr(error));
}

CHIP_ERROR ExchangeManager::RegisterUMH(Protocols::Id protocolId, int16_t msgType, ExchangeDelegateBase * delegate)
CHIP_ERROR ExchangeManager::RegisterUMH(Protocols::Id protocolId, int16_t msgType, ExchangeDelegate * delegate)
{
UnsolicitedMessageHandler * selected = nullptr;

Expand Down Expand Up @@ -337,7 +338,7 @@ void ExchangeManager::OnMessageReceived(const Transport::PeerAddress & source, S
}
}

void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegateBase * delegate)
void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * delegate)
{
mContextPool.ForEachActiveObject([&](auto * ec) {
if (ec->GetDelegate() == delegate)
Expand Down
18 changes: 10 additions & 8 deletions src/messaging/ExchangeMgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace chip {
namespace Messaging {

class ExchangeContext;
class ExchangeDelegateBase;
class ExchangeDelegate;

static constexpr int16_t kAnyMessageType = -1;

Expand Down Expand Up @@ -99,7 +99,7 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
* @return A pointer to the created ExchangeContext object On success. Otherwise NULL if no object
* can be allocated or is available.
*/
ExchangeContext * NewContext(SecureSessionHandle session, ExchangeDelegateBase * delegate);
ExchangeContext * NewContext(SecureSessionHandle session, ExchangeDelegate * delegate);

void ReleaseContext(ExchangeContext * ec) { mContextPool.ReleaseObject(ec); }

Expand All @@ -115,7 +115,7 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
* is full and a new one cannot be allocated.
* @retval #CHIP_NO_ERROR On success.
*/
CHIP_ERROR RegisterUnsolicitedMessageHandlerForProtocol(Protocols::Id protocolId, ExchangeDelegateBase * delegate);
CHIP_ERROR RegisterUnsolicitedMessageHandlerForProtocol(Protocols::Id protocolId, ExchangeDelegate * delegate);

/**
* Register an unsolicited message handler for a given protocol identifier and message type.
Expand All @@ -130,13 +130,13 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
* is full and a new one cannot be allocated.
* @retval #CHIP_NO_ERROR On success.
*/
CHIP_ERROR RegisterUnsolicitedMessageHandlerForType(Protocols::Id protocolId, uint8_t msgType, ExchangeDelegateBase * delegate);
CHIP_ERROR RegisterUnsolicitedMessageHandlerForType(Protocols::Id protocolId, uint8_t msgType, ExchangeDelegate * delegate);

/**
* A strongly-message-typed version of RegisterUnsolicitedMessageHandlerForType.
*/
template <typename MessageType, typename = std::enable_if_t<std::is_enum<MessageType>::value>>
CHIP_ERROR RegisterUnsolicitedMessageHandlerForType(MessageType msgType, ExchangeDelegateBase * delegate)
CHIP_ERROR RegisterUnsolicitedMessageHandlerForType(MessageType msgType, ExchangeDelegate * delegate)
{
static_assert(std::is_same<std::underlying_type_t<MessageType>, uint8_t>::value, "Enum is wrong size; cast is not safe");
return RegisterUnsolicitedMessageHandlerForType(Protocols::MessageTypeTraits<MessageType>::ProtocolId(),
Expand Down Expand Up @@ -183,7 +183,7 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
* their delegate. To be used if the delegate is being destroyed. This
* method will guarantee that it does not call into the delegate.
*/
void CloseAllContextsForDelegate(const ExchangeDelegateBase * delegate);
void CloseAllContextsForDelegate(const ExchangeDelegate * delegate);

void SetDelegate(ExchangeMgrDelegate * delegate) { mDelegate = delegate; }

Expand Down Expand Up @@ -214,7 +214,7 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
return ProtocolId == aProtocolId && MessageType == aMessageType;
}

ExchangeDelegateBase * Delegate;
ExchangeDelegate * Delegate;
Protocols::Id ProtocolId;
// Message types are normally 8-bit unsigned ints, but we use
// kAnyMessageType, which is negative, to represent a wildcard handler,
Expand All @@ -231,13 +231,15 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
SecureSessionMgr * mSessionMgr;
ReliableMessageMgr mReliableMessageMgr;

ApplicationExchangeDispatch mDefaultExchangeDispatch;

Transport::AdminId mAdminId = 0;

BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> mContextPool;

UnsolicitedMessageHandler UMHandlerPool[CHIP_CONFIG_MAX_UNSOLICITED_MESSAGE_HANDLERS];

CHIP_ERROR RegisterUMH(Protocols::Id protocolId, int16_t msgType, ExchangeDelegateBase * delegate);
CHIP_ERROR RegisterUMH(Protocols::Id protocolId, int16_t msgType, ExchangeDelegate * delegate);
CHIP_ERROR UnregisterUMH(Protocols::Id protocolId, int16_t msgType);

void OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source, SecureSessionMgr * msgLayer) override;
Expand Down
4 changes: 2 additions & 2 deletions src/messaging/tests/MessagingContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,13 @@ SecureSessionHandle MessagingContext::GetSessionPeerToLocal()
return { GetSourceNodeId(), GetLocalKeyId(), GetAdminId() };
}

Messaging::ExchangeContext * MessagingContext::NewExchangeToPeer(Messaging::ExchangeDelegateBase * delegate)
Messaging::ExchangeContext * MessagingContext::NewExchangeToPeer(Messaging::ExchangeDelegate * delegate)
{
// TODO: temprary create a SecureSessionHandle from node id, will be fix in PR 3602
return mExchangeManager.NewContext(GetSessionLocalToPeer(), delegate);
}

Messaging::ExchangeContext * MessagingContext::NewExchangeToLocal(Messaging::ExchangeDelegateBase * delegate)
Messaging::ExchangeContext * MessagingContext::NewExchangeToLocal(Messaging::ExchangeDelegate * delegate)
{
// TODO: temprary create a SecureSessionHandle from node id, will be fix in PR 3602
return mExchangeManager.NewContext(GetSessionPeerToLocal(), delegate);
Expand Down
4 changes: 2 additions & 2 deletions src/messaging/tests/MessagingContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ class MessagingContext : public IOContext
SecureSessionHandle GetSessionLocalToPeer();
SecureSessionHandle GetSessionPeerToLocal();

Messaging::ExchangeContext * NewExchangeToPeer(Messaging::ExchangeDelegateBase * delegate);
Messaging::ExchangeContext * NewExchangeToLocal(Messaging::ExchangeDelegateBase * delegate);
Messaging::ExchangeContext * NewExchangeToPeer(Messaging::ExchangeDelegate * delegate);
Messaging::ExchangeContext * NewExchangeToLocal(Messaging::ExchangeDelegate * delegate);

Credentials::OperationalCredentialSet & GetOperationalCredentialSet() { return mOperationalCredentialSet; }

Expand Down
Loading

0 comments on commit 7ce0fe3

Please sign in to comment.