Skip to content

Commit

Permalink
Refactor/Split ExchangeMgrDelegate (#11789)
Browse files Browse the repository at this point in the history
  • Loading branch information
kghost authored and pull[bot] committed Mar 9, 2023
1 parent 6b7a1c5 commit 4255265
Show file tree
Hide file tree
Showing 22 changed files with 278 additions and 248 deletions.
12 changes: 3 additions & 9 deletions src/app/CASESessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,18 +111,12 @@ CHIP_ERROR CASESessionManager::GetPeerAddress(NodeId nodeId, Transport::PeerAddr
return CHIP_NO_ERROR;
}

void CASESessionManager::OnNewConnection(SessionHandle sessionHandle, Messaging::ExchangeManager * mgr)
{
// TODO Update the MRP params based on the MRP params extracted from CASE, when this is available.
}

void CASESessionManager::OnConnectionExpired(SessionHandle sessionHandle, Messaging::ExchangeManager * mgr)
void CASESessionManager::OnSessionReleased(SessionHandle sessionHandle)
{
OperationalDeviceProxy * session = FindSession(sessionHandle);
VerifyOrReturn(session != nullptr,
ChipLogDetail(Controller, "OnConnectionExpired was called for unknown device, ignoring it."));
VerifyOrReturn(session != nullptr, ChipLogDetail(Controller, "OnSessionReleased was called for unknown device, ignoring it."));

session->OnConnectionExpired(sessionHandle);
session->OnSessionReleased(sessionHandle);
}

OperationalDeviceProxy * CASESessionManager::FindSession(SessionHandle session)
Expand Down
9 changes: 4 additions & 5 deletions src/app/CASESessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#include <lib/core/CHIPCore.h>
#include <lib/dnssd/DnssdCache.h>
#include <lib/support/Pool.h>
#include <messaging/ExchangeMgrDelegate.h>
#include <transport/SessionDelegate.h>

#include <lib/dnssd/Resolver.h>

Expand All @@ -43,7 +43,7 @@ struct CASESessionManagerConfig
* 4. During session establishment, trigger node ID resolution (if needed), and update the DNS-SD cache (if resolution is
* successful)
*/
class CASESessionManager : public Messaging::ExchangeMgrDelegate, public Dnssd::ResolverDelegate
class CASESessionManager : public SessionReleaseDelegate, public Dnssd::ResolverDelegate
{
public:
CASESessionManager() = delete;
Expand Down Expand Up @@ -91,9 +91,8 @@ class CASESessionManager : public Messaging::ExchangeMgrDelegate, public Dnssd::
*/
CHIP_ERROR GetPeerAddress(NodeId nodeId, Transport::PeerAddress & addr);

//////////// ExchangeMgrDelegate Implementation ///////////////
void OnNewConnection(SessionHandle session, Messaging::ExchangeManager * mgr) override;
void OnConnectionExpired(SessionHandle session, Messaging::ExchangeManager * mgr) override;
//////////// SessionReleaseDelegate Implementation ///////////////
void OnSessionReleased(SessionHandle session) override;

//////////// ResolverDelegate Implementation ///////////////
void OnNodeIdResolved(const Dnssd::ResolvedNodeData & nodeData) override;
Expand Down
6 changes: 0 additions & 6 deletions src/app/DeviceProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,6 @@ class DLL_EXPORT DeviceProxy
virtual ~DeviceProxy() {}
DeviceProxy() {}

/**
* Called when a connection is closing.
* The object releases all resources associated with the connection.
*/
virtual void OnConnectionExpired(SessionHandle session) = 0;

/**
* Mark any open session with the device as expired.
*/
Expand Down
3 changes: 2 additions & 1 deletion src/app/OperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ void OperationalDeviceProxy::OnSessionEstablished()
VerifyOrReturn(mState != State::Uninitialized,
ChipLogError(Controller, "OnSessionEstablished was called while the device was not initialized"));

// TODO Update the MRP params based on the MRP params extracted from CASE, when this is available.
CHIP_ERROR err = mInitParams.sessionManager->NewPairing(
Optional<Transport::PeerAddress>::Value(mDeviceAddress), mPeerId.GetNodeId(), &mCASESession,
CryptoContext::SessionRole::kInitiator, mInitParams.fabricInfo->GetFabricIndex());
Expand Down Expand Up @@ -267,7 +268,7 @@ void OperationalDeviceProxy::Clear()
mInitParams = DeviceProxyInitParams();
}

void OperationalDeviceProxy::OnConnectionExpired(SessionHandle session)
void OperationalDeviceProxy::OnSessionReleased(SessionHandle session)
{
VerifyOrReturn(mSecureSession.HasValue() && mSecureSession.Value() == session,
ChipLogDetail(Controller, "Connection expired, but it doesn't match the current session"));
Expand Down
4 changes: 2 additions & 2 deletions src/app/OperationalDeviceProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class OperationalDeviceProxy;
typedef void (*OnDeviceConnected)(void * context, DeviceProxy * device);
typedef void (*OnDeviceConnectionFailure)(void * context, NodeId deviceId, CHIP_ERROR error);

class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, public SessionEstablishmentDelegate
class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, SessionReleaseDelegate, public SessionEstablishmentDelegate
{
public:
virtual ~OperationalDeviceProxy();
Expand Down Expand Up @@ -113,7 +113,7 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, public SessionEsta
* Called when a connection is closing.
* The object releases all resources associated with the connection.
*/
void OnConnectionExpired(SessionHandle session) override;
void OnSessionReleased(SessionHandle session) override;

void OnNodeIdResolved(const Dnssd::ResolvedNodeData & nodeResolutionData)
{
Expand Down
23 changes: 7 additions & 16 deletions src/channel/Manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@ class ChannelContext;
* @brief
* This class is used to manage Channel Contexts with other CHIP nodes.
*/
class DLL_EXPORT ChannelManager : public ExchangeMgrDelegate
class DLL_EXPORT ChannelManager : public SessionCreationDelegate
{
public:
ChannelManager(ExchangeManager * exchangeManager) : mExchangeManager(exchangeManager) { exchangeManager->SetDelegate(this); }
ChannelManager(ExchangeManager * exchangeManager) : mExchangeManager(exchangeManager)
{
exchangeManager->GetSessionManager()->RegisterCreationDelegate(*this);
}
ChannelManager(const ChannelManager &) = delete;
ChannelManager operator=(const ChannelManager &) = delete;

Expand All @@ -58,10 +61,10 @@ class DLL_EXPORT ChannelManager : public ExchangeMgrDelegate
});
}

void OnNewConnection(SessionHandle session, ExchangeManager * mgr) override
void OnNewSession(SessionHandle session) override
{
mChannelContexts.ForEachActiveObject([&](ChannelContext * context) {
if (context->MatchesSession(session, mgr->GetSessionManager()))
if (context->MatchesSession(session, mExchangeManager->GetSessionManager()))
{
context->OnNewConnection(session);
return false;
Expand All @@ -70,18 +73,6 @@ class DLL_EXPORT ChannelManager : public ExchangeMgrDelegate
});
}

void OnConnectionExpired(SessionHandle session, ExchangeManager * mgr) override
{
mChannelContexts.ForEachActiveObject([&](ChannelContext * context) {
if (context->MatchesSession(session, mgr->GetSessionManager()))
{
context->OnConnectionExpired(session);
return false;
}
return true;
});
}

private:
BitMapObjectPool<ChannelContext, CHIP_CONFIG_MAX_ACTIVE_CHANNELS> mChannelContexts;
BitMapObjectPool<ChannelContextHandleAssociation, CHIP_CONFIG_MAX_CHANNEL_HANDLES> mChannelHandles;
Expand Down
20 changes: 10 additions & 10 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,6 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)

VerifyOrReturnError(params.systemState->TransportMgr() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

// TODO Exchange Mgr needs to be able to track multiple delegates. Delegate API should be able to query for the right delegate
// to handle events.
params.systemState->ExchangeMgr()->SetDelegate(this);

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Dnssd::Resolver::Instance().Init(params.systemState->InetLayer());
Dnssd::Resolver::Instance().SetResolverDelegate(this);
Expand Down Expand Up @@ -266,10 +262,10 @@ void DeviceController::ReleaseOperationalDevice(NodeId remoteDeviceId)
mCASESessionManager->ReleaseSession(remoteDeviceId);
}

void DeviceController::OnConnectionExpired(SessionHandle session, Messaging::ExchangeManager * mgr)
void DeviceController::OnSessionReleased(SessionHandle session)
{
VerifyOrReturn(mState == State::Initialized, ChipLogError(Controller, "OnConnectionExpired was called in incorrect state"));
mCASESessionManager->OnConnectionExpired(session, mgr);
mCASESessionManager->OnSessionReleased(session);
}

CHIP_ERROR DeviceController::InitializePairedDeviceList()
Expand Down Expand Up @@ -544,6 +540,9 @@ CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)
{
ReturnErrorOnFailure(DeviceController::Init(params));

params.systemState->SessionMgr()->RegisterCreationDelegate(*this);
params.systemState->SessionMgr()->RegisterReleaseDelegate(*this);

uint16_t nextKeyID = 0;
uint16_t size = sizeof(nextKeyID);
CHIP_ERROR error = mStorageDelegate->SyncGetKeyValue(kNextAvailableKeyID, &nextKeyID, size);
Expand Down Expand Up @@ -605,24 +604,25 @@ CHIP_ERROR DeviceCommissioner::Shutdown()
return CHIP_NO_ERROR;
}

void DeviceCommissioner::OnNewConnection(SessionHandle session, Messaging::ExchangeManager * mgr)
void DeviceCommissioner::OnNewSession(SessionHandle session)
{
VerifyOrReturn(mState == State::Initialized, ChipLogError(Controller, "OnNewConnection was called in incorrect state"));

CommissioneeDeviceProxy * device = FindCommissioneeDevice(mgr->GetSessionManager()->GetSecureSession(session)->GetPeerNodeId());
CommissioneeDeviceProxy * device =
FindCommissioneeDevice(mSystemState->SessionMgr()->GetSecureSession(session)->GetPeerNodeId());
VerifyOrReturn(device != nullptr, ChipLogDetail(Controller, "OnNewConnection was called for unknown device, ignoring it."));

device->OnNewConnection(session);
}

void DeviceCommissioner::OnConnectionExpired(SessionHandle session, Messaging::ExchangeManager * mgr)
void DeviceCommissioner::OnSessionReleased(SessionHandle session)
{
VerifyOrReturn(mState == State::Initialized, ChipLogError(Controller, "OnConnectionExpired was called in incorrect state"));

CommissioneeDeviceProxy * device = FindCommissioneeDevice(session);
VerifyOrReturn(device != nullptr, ChipLogDetail(Controller, "OnConnectionExpired was called for unknown device, ignoring it."));

device->OnConnectionExpired(session);
device->OnSessionReleased(session);
}

CommissioneeDeviceProxy * DeviceCommissioner::FindCommissioneeDevice(SessionHandle session)
Expand Down
17 changes: 8 additions & 9 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
#include <lib/support/SerializableIntegerSet.h>
#include <lib/support/Span.h>
#include <messaging/ExchangeMgr.h>
#include <messaging/ExchangeMgrDelegate.h>
#include <protocols/secure_channel/MessageCounterManager.h>
#include <protocols/secure_channel/RendezvousParameters.h>
#include <protocols/user_directed_commissioning/UserDirectedCommissioning.h>
Expand Down Expand Up @@ -187,7 +186,7 @@ typedef void (*OnOpenCommissioningWindow)(void * context, NodeId deviceId, CHIP_
* and device pairing information for individual devices). Alternatively, this class can retrieve the
* relevant information when the application tries to communicate with the device
*/
class DLL_EXPORT DeviceController : public Messaging::ExchangeMgrDelegate,
class DLL_EXPORT DeviceController : public SessionReleaseDelegate,
#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
public AbstractDnssdDiscoveryController,
#endif
Expand Down Expand Up @@ -376,9 +375,8 @@ class DLL_EXPORT DeviceController : public Messaging::ExchangeMgrDelegate,

uint16_t mVendorId;

//////////// ExchangeMgrDelegate Implementation ///////////////
void OnNewConnection(SessionHandle session, Messaging::ExchangeManager * mgr) override {}
void OnConnectionExpired(SessionHandle session, Messaging::ExchangeManager * mgr) override;
//////////// SessionReleaseDelegate Implementation ///////////////
void OnSessionReleased(SessionHandle session) override;

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
//////////// ResolverDelegate Implementation ///////////////
Expand Down Expand Up @@ -412,12 +410,12 @@ class DLL_EXPORT DeviceController : public Messaging::ExchangeMgrDelegate,
* will be stored.
*/
class DLL_EXPORT DeviceCommissioner : public DeviceController,
public SessionCreationDelegate,
#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable
public Protocols::UserDirectedCommissioning::InstanceNameResolver,
public Protocols::UserDirectedCommissioning::UserConfirmationProvider,
#endif
public SessionEstablishmentDelegate

{
public:
DeviceCommissioner();
Expand Down Expand Up @@ -624,9 +622,10 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,

void OnSessionEstablishmentTimeout();

//////////// ExchangeMgrDelegate Implementation ///////////////
void OnNewConnection(SessionHandle session, Messaging::ExchangeManager * mgr) override;
void OnConnectionExpired(SessionHandle session, Messaging::ExchangeManager * mgr) override;
//////////// SessionCreationDelegate Implementation ///////////////
void OnNewSession(SessionHandle session) override;
//////////// SessionReleaseDelegate Implementation ///////////////
void OnSessionReleased(SessionHandle session) override;

static void OnSessionEstablishmentTimeoutCallback(System::Layer * aLayer, void * aAppState);

Expand Down
2 changes: 1 addition & 1 deletion src/controller/CommissioneeDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void CommissioneeDeviceProxy::OnNewConnection(SessionHandle session)
mSecureSession.SetValue(session);
}

void CommissioneeDeviceProxy::OnConnectionExpired(SessionHandle session)
void CommissioneeDeviceProxy::OnSessionReleased(SessionHandle session)
{
VerifyOrReturn(mSecureSession.HasValue() && mSecureSession.Value() == session,
ChipLogDetail(Controller, "Connection expired, but it doesn't match the current session"));
Expand Down
6 changes: 3 additions & 3 deletions src/controller/CommissioneeDeviceProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ struct ControllerDeviceInitParams
Controller::DeviceControllerInteractionModelDelegate * imDelegate = nullptr;
};

class CommissioneeDeviceProxy : public DeviceProxy
class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegate
{
public:
~CommissioneeDeviceProxy();
Expand Down Expand Up @@ -165,13 +165,13 @@ class CommissioneeDeviceProxy : public DeviceProxy

/**
* @brief
* Called when a connection is closing.
* Called when the associated session is released
*
* The receiver should release all resources associated with the connection.
*
* @param session A handle to the secure session
*/
void OnConnectionExpired(SessionHandle session) override;
void OnSessionReleased(SessionHandle session) override;

/**
* In case there exists an open session to the device, mark it as expired.
Expand Down
4 changes: 2 additions & 2 deletions src/controller/tests/data_model/TestRead.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ void TestReadInteraction::TestReadTimeout(nlTestSuite * apSuite, void * apContex
NL_TEST_ASSERT(apSuite, chip::app::InteractionModelEngine::GetInstance()->GetNumActiveReadClients() == 1);
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 2);

ctx.GetExchangeManager().OnConnectionExpired(ctx.GetSessionBobToAlice());
ctx.GetExchangeManager().ExpireExchangesForSession(ctx.GetSessionBobToAlice());

ctx.DrainAndServiceIO();

Expand All @@ -251,7 +251,7 @@ void TestReadInteraction::TestReadTimeout(nlTestSuite * apSuite, void * apContex
chip::app::InteractionModelEngine::GetInstance()->GetReportingEngine().Run();
ctx.DrainAndServiceIO();

ctx.GetExchangeManager().OnConnectionExpired(ctx.GetSessionAliceToBob());
ctx.GetExchangeManager().ExpireExchangesForSession(ctx.GetSessionAliceToBob());

NL_TEST_ASSERT(apSuite, chip::app::InteractionModelEngine::GetInstance()->GetNumActiveReadHandlers() == 0);

Expand Down
18 changes: 18 additions & 0 deletions src/lib/core/CHIPConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -2673,6 +2673,24 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
"Please enable at least one of CHIP_CONFIG_EXAMPLE_ACCESS_CONTROL_FAST_COPY_SUPPORT or CHIP_CONFIG_EXAMPLE_ACCESS_CONTROL_FLEXIBLE_COPY_SUPPORT"
#endif

/**
* @def CHIP_CONFIG_MAX_SESSION_CREATION_DELEGATES
*
* @brief Defines the max number of SessionCreationDelegates
*/
#ifndef CHIP_CONFIG_MAX_SESSION_CREATION_DELEGATES
#define CHIP_CONFIG_MAX_SESSION_CREATION_DELEGATES 2
#endif

/**
* @def CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES
*
* @brief Defines the max number of SessionReleaseDelegate
*/
#ifndef CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES
#define CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES 2
#endif

/**
* @}
*/
1 change: 0 additions & 1 deletion src/messaging/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ static_library("messaging") {
"ExchangeMessageDispatch.h",
"ExchangeMgr.cpp",
"ExchangeMgr.h",
"ExchangeMgrDelegate.h",
"Flags.h",
"ReliableMessageContext.cpp",
"ReliableMessageContext.h",
Expand Down
Loading

0 comments on commit 4255265

Please sign in to comment.