Skip to content

Commit

Permalink
Use RAII for ExchangeContext contruction and destruction instead of A…
Browse files Browse the repository at this point in the history 
…lloc/Free. (#6808)
  • Loading branch information
@kghost @pull
kghost authored and pull[bot] committed Jun 4, 2021
1 parent b3f4097 commit 1481398
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 118 deletions.
28 changes: 9 additions & 19 deletions src/messaging/ExchangeContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,9 @@ void ExchangeContext::Abort()
Release();
}

void ExchangeContext::Reset()
void ExchangeContextDeletor::Release(ExchangeContext * ec)
{
*this = ExchangeContext();
ec->mExchangeMgr->ReleaseContext(ec);
}

ExchangeMessageDispatch * ExchangeContext::GetMessageDispatch()
Expand All @@ -227,15 +227,12 @@ ExchangeMessageDispatch * ExchangeContext::GetMessageDispatch()
return nullptr;
}

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

Reset();
Retain();
mExchangeMgr = em;
em->IncrementContextsInUse();
mExchangeMgr = em;
mExchangeId = ExchangeId;
mSecureSession = session;
mFlags.Set(Flags::kFlagInitiator, Initiator);
Expand All @@ -248,37 +245,30 @@ ExchangeContext * ExchangeContext::Alloc(ExchangeManager * em, uint16_t Exchange
SetAutoRequestAck(true);

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
ChipLogDetail(ExchangeManager, "ec++ id: %d, inUse: %d, addr: 0x%x", (this - em->mContextPool.begin()), em->GetContextsInUse(),
this);
ChipLogDetail(ExchangeManager, "ec++ id: %d", ExchangeId);
#endif
SYSTEM_STATS_INCREMENT(chip::System::Stats::kExchangeMgr_NumContexts);

return this;
}

void ExchangeContext::Free()
ExchangeContext::~ExchangeContext()
{
VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0);

// Ideally, in this scenario, the retransmit table should
// be clear of any outstanding messages for this context and
// the boolean parameter passed to DoClose() should not matter.
ExchangeManager * em = mExchangeMgr;

DoClose(false);
mExchangeMgr = nullptr;

em->DecrementContextsInUse();

if (mExchangeACL != nullptr)
{
chip::Platform::Delete(mExchangeACL);
mExchangeACL = nullptr;
}

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
ChipLogDetail(ExchangeManager, "ec-- id: %d [%04" PRIX16 "], inUse: %d, addr: 0x%x", (this - em->mContextPool.begin()),
mExchangeId, em->GetContextsInUse(), this);
ChipLogDetail(ExchangeManager, "ec-- id: %d", mExchangeId);
#endif
SYSTEM_STATS_DECREMENT(chip::System::Stats::kExchangeMgr_NumContexts);
}
Expand Down
18 changes: 6 additions & 12 deletions src/messaging/ExchangeContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,19 @@ class ExchangeContextDeletor
* It defines methods for encoding and communicating CHIP messages within an ExchangeContext
* over various transport mechanisms, for example, TCP, UDP, or CHIP Reliable Messaging.
*/
class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
public ReferenceCounted<ExchangeContext, ExchangeContextDeletor, 0>
class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public ReferenceCounted<ExchangeContext, ExchangeContextDeletor>
{
friend class ExchangeManager;
friend class ExchangeContextDeletor;

public:
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);

~ExchangeContext();

/**
* Determine whether the context is the initiator of the exchange.
*
Expand Down Expand Up @@ -170,11 +174,6 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
SecureSessionHandle mSecureSession; // The connection state
uint16_t mExchangeId; // Assigned exchange ID.

ExchangeContext * Alloc(ExchangeManager * em, uint16_t ExchangeId, SecureSessionHandle session, bool Initiator,
ExchangeDelegateBase * delegate);
void Free();
void Reset();

/**
* Determine whether a response is currently expected for a message that was sent over
* this exchange. While this is true, attempts to send other messages that expect a response
Expand Down Expand Up @@ -216,10 +215,5 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
void DoClose(bool clearRetransTable);
};

inline void ExchangeContextDeletor::Release(ExchangeContext * obj)
{
obj->Free();
}

} // namespace Messaging
} // namespace chip
109 changes: 38 additions & 71 deletions src/messaging/ExchangeMgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,6 @@ CHIP_ERROR ExchangeManager::Init(SecureSessionMgr * sessionMgr)
mNextExchangeId = GetRandU16();
mNextKeyId = 0;

mContextsInUse = 0;

for (auto & handler : UMHandlerPool)
{
// Mark all handlers as unallocated. This handles both initial
Expand All @@ -98,11 +96,11 @@ CHIP_ERROR ExchangeManager::Shutdown()
{
mReliableMessageMgr.Shutdown();

for (auto & ec : mContextPool)
{
// ExchangeContext leaked
assert(ec.GetReferenceCount() == 0);
}
mContextPool.ForEachActiveObject([](auto * ec) {
// There should be no active object in the pool
assert(false);
return true;
});

if (mSessionMgr != nullptr)
{
Expand All @@ -117,7 +115,7 @@ CHIP_ERROR ExchangeManager::Shutdown()

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

CHIP_ERROR ExchangeManager::RegisterUnsolicitedMessageHandlerForProtocol(Protocols::Id protocolId, ExchangeDelegateBase * delegate)
Expand Down Expand Up @@ -146,23 +144,6 @@ void ExchangeManager::OnReceiveError(CHIP_ERROR error, const Transport::PeerAddr
ChipLogError(ExchangeManager, "Accept FAILED, err = %s", ErrorStr(error));
}

ExchangeContext * ExchangeManager::AllocContext(uint16_t ExchangeId, SecureSessionHandle session, bool Initiator,
ExchangeDelegateBase * delegate)
{
CHIP_FAULT_INJECT(FaultInjection::kFault_AllocExchangeContext, return nullptr);

for (auto & ec : mContextPool)
{
if (ec.GetReferenceCount() == 0)
{
return ec.Alloc(this, ExchangeId, session, Initiator, delegate);
}
}

ChipLogError(ExchangeManager, "Alloc ctxt FAILED");
return nullptr;
}

CHIP_ERROR ExchangeManager::RegisterUMH(Protocols::Id protocolId, int16_t msgType, ExchangeDelegateBase * delegate)
{
UnsolicitedMessageHandler * umh = UMHandlerPool;
Expand Down Expand Up @@ -224,22 +205,28 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
payloadHeader.GetProtocolID());

// Search for an existing exchange that the message applies to. If a match is found...
for (auto & ec : mContextPool)
{
if (ec.GetReferenceCount() > 0 && ec.MatchExchange(session, packetHeader, payloadHeader))
bool found = false;
mContextPool.ForEachActiveObject([&](auto * ec) {
if (ec->MatchExchange(session, packetHeader, payloadHeader))
{
// Found a matching exchange. Set flag for correct subsequent CRMP
// retransmission timeout selection.
if (!ec.HasRcvdMsgFromPeer())
if (!ec->HasRcvdMsgFromPeer())
{
ec.SetMsgRcvdFromPeer(true);
ec->SetMsgRcvdFromPeer(true);
}

// Matched ExchangeContext; send to message handler.
ec.HandleMessage(packetHeader, payloadHeader, source, std::move(msgBuf));

ExitNow(err = CHIP_NO_ERROR);
ec->HandleMessage(packetHeader, payloadHeader, source, std::move(msgBuf));
found = true;
return false;
}
return true;
});

if (found)
{
ExitNow(err = CHIP_NO_ERROR);
}

// Search for an unsolicited message handler if it marked as being sent by an initiator. Since we didn't
Expand Down Expand Up @@ -289,17 +276,16 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
// If rcvd msg is from initiator then this exchange is created as not Initiator.
// If rcvd msg is not from initiator then this exchange is created as Initiator.
// TODO: Figure out which channel to use for the received message
ec = AllocContext(payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), nullptr);
ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), nullptr);
}
else
{
ec = AllocContext(payloadHeader.GetExchangeID(), session, false, matchingUMH->Delegate);
ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, false, matchingUMH->Delegate);
}

VerifyOrExit(ec != nullptr, err = CHIP_ERROR_NO_MEMORY);

ChipLogDetail(ExchangeManager, "ec pos: %d, id: %d, Delegate: 0x%x", ec - mContextPool.begin(), ec->GetExchangeId(),
ec->GetDelegate());
ChipLogDetail(ExchangeManager, "ec id: %d, Delegate: 0x%x", ec->GetExchangeId(), ec->GetDelegate());

ec->HandleMessage(packetHeader, payloadHeader, source, std::move(msgBuf));

Expand Down Expand Up @@ -330,14 +316,14 @@ void ExchangeManager::OnConnectionExpired(SecureSessionHandle session, SecureSes
mDelegate->OnConnectionExpired(session, this);
}

for (auto & ec : mContextPool)
{
if (ec.GetReferenceCount() > 0 && ec.mSecureSession == session)
mContextPool.ForEachActiveObject([&](auto * ec) {
if (ec->mSecureSession == session)
{
ec.Close();
ec->Close();
// Continue iterate because there can be multiple contexts associated with the connection.
}
}
return true;
});
}

void ExchangeManager::OnMessageReceived(const Transport::PeerAddress & source, System::PacketBufferHandle msgBuf)
Expand All @@ -359,37 +345,18 @@ void ExchangeManager::OnMessageReceived(const Transport::PeerAddress & source, S

void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegateBase * delegate)
{
for (auto & ec : mContextPool)
{
if (ec.GetReferenceCount() == 0 || ec.GetDelegate() != delegate)
mContextPool.ForEachActiveObject([&](auto * ec) {
if (ec->GetDelegate() == delegate)
{
continue;
// Make sure to null out the delegate before closing the context, so
// we don't notify the delegate that the context is closing. We
// have to do this, because the delegate might be partially
// destroyed by this point.
ec->SetDelegate(nullptr);
ec->Close();
}

// Make sure to null out the delegate before closing the context, so
// we don't notify the delegate that the context is closing. We
// have to do this, because the delegate might be partially
// destroyed by this point.
ec.SetDelegate(nullptr);
ec.Close();
}
}

void ExchangeManager::IncrementContextsInUse()
{
mContextsInUse++;
}

void ExchangeManager::DecrementContextsInUse()
{
if (mContextsInUse >= 1)
{
mContextsInUse--;
}
else
{
ChipLogError(ExchangeManager, "No context in use, decrement failed");
}
return true;
});
}

} // namespace Messaging
Expand Down
13 changes: 4 additions & 9 deletions src/messaging/ExchangeMgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <messaging/ReliableMessageMgr.h>
#include <protocols/Protocols.h>
#include <support/DLLUtil.h>
#include <support/Pool.h>
#include <transport/SecureSessionMgr.h>
#include <transport/TransportMgr.h>

Expand Down Expand Up @@ -100,6 +101,8 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
*/
ExchangeContext * NewContext(SecureSessionHandle session, ExchangeDelegateBase * delegate);

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

/**
* Register an unsolicited message handler for a given protocol identifier. This handler would be
* invoked for all messages of the given protocol.
Expand Down Expand Up @@ -182,9 +185,6 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
*/
void CloseAllContextsForDelegate(const ExchangeDelegateBase * delegate);

void IncrementContextsInUse();
void DecrementContextsInUse();

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

SecureSessionMgr * GetSessionMgr() const { return mSessionMgr; }
Expand All @@ -194,7 +194,6 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
Transport::AdminId GetAdminId() { return mAdminId; }

uint16_t GetNextKeyId() { return ++mNextKeyId; }
size_t GetContextsInUse() const { return mContextsInUse; }

private:
enum class State
Expand Down Expand Up @@ -234,14 +233,10 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans

Transport::AdminId mAdminId = 0;

std::array<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> mContextPool;
size_t mContextsInUse;
BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> mContextPool;

UnsolicitedMessageHandler UMHandlerPool[CHIP_CONFIG_MAX_UNSOLICITED_MESSAGE_HANDLERS];

ExchangeContext * AllocContext(uint16_t ExchangeId, SecureSessionHandle session, bool Initiator,
ExchangeDelegateBase * delegate);

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

Expand Down
2 changes: 1 addition & 1 deletion src/messaging/ReliableMessageMgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace Messaging {

ReliableMessageMgr::RetransTableEntry::RetransTableEntry() : rc(nullptr), nextRetransTimeTick(0), sendCount(0) {}

ReliableMessageMgr::ReliableMessageMgr(std::array<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & contextPool) :
ReliableMessageMgr::ReliableMessageMgr(BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & contextPool) :
mContextPool(contextPool), mSystemLayer(nullptr), mSessionMgr(nullptr), mCurrentTimerExpiry(0),
mTimerIntervalShift(CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT)
{}
Expand Down
Loading

0 comments on commit 1481398

Please sign in to comment.