Skip to content

Commit

Permalink
Added automatic node resolve after first transmission failure. (#12263)
Browse files Browse the repository at this point in the history
* Data advertised in the DNS-SD records such as IP address
or CRA/CRI values may change in time and it may result
in communication failures.
* DNS-SD records are cached, so in case of device IP address
change and trying to resolve address again the cached value
is used instead of the new one.

* Added argument that allows to bypass reading DNS-SD services
data from cache and force resolving node data.
* Added SessionRecoveryDelegate that can be used to notify
about the first message transmission failure.
* Added CHIPDeviceController implementation of
SessionRecoveryDelegate to trigger resolving node data again
if communication failure appeared.
  • Loading branch information
kkasperczyk-no authored Nov 30, 2021
1 parent 2278e6b commit 3958429
Show file tree
Hide file tree
Showing 16 changed files with 121 additions and 15 deletions.
3 changes: 2 additions & 1 deletion examples/chip-tool/commands/discover/Commands.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class Resolve : public DiscoverCommand, public chip::Dnssd::ResolverDelegate
chip::Dnssd::Resolver::Instance().SetResolverDelegate(this);
ChipLogProgress(chipTool, "Dnssd: Searching for NodeId: %" PRIx64 " FabricId: %" PRIx64 " ...", remoteId, fabricId);
return chip::Dnssd::Resolver::Instance().ResolveNodeId(chip::PeerId().SetNodeId(remoteId).SetCompressedFabricId(fabricId),
chip::Inet::IPAddressType::kAny);
chip::Inet::IPAddressType::kAny,
chip::Dnssd::Resolver::CacheBypass::On);
}

void OnNodeIdResolved(const chip::Dnssd::ResolvedNodeData & nodeData) override
Expand Down
3 changes: 2 additions & 1 deletion src/app/CASESessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ void CASESessionManager::ReleaseSession(NodeId nodeId)

CHIP_ERROR CASESessionManager::ResolveDeviceAddress(NodeId nodeId)
{
return Dnssd::Resolver::Instance().ResolveNodeId(GetFabricInfo()->GetPeerIdForNode(nodeId), Inet::IPAddressType::kAny);
return Dnssd::Resolver::Instance().ResolveNodeId(GetFabricInfo()->GetPeerIdForNode(nodeId), Inet::IPAddressType::kAny,
Dnssd::Resolver::CacheBypass::On);
}

void CASESessionManager::OnNodeIdResolved(const Dnssd::ResolvedNodeData & nodeData)
Expand Down
8 changes: 8 additions & 0 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,13 @@ void DeviceController::OnSessionReleased(SessionHandle session)
mCASESessionManager->OnSessionReleased(session);
}

void DeviceController::OnFirstMessageDeliveryFailed(const SessionHandle & session)
{
VerifyOrReturn(mState == State::Initialized,
ChipLogError(Controller, "OnFirstMessageDeliveryFailed was called in incorrect state"));
UpdateDevice(session.GetPeerNodeId());
}

CHIP_ERROR DeviceController::InitializePairedDeviceList()
{
CHIP_ERROR err = CHIP_NO_ERROR;
Expand Down Expand Up @@ -543,6 +550,7 @@ CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)

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

uint16_t nextKeyID = 0;
uint16_t size = sizeof(nextKeyID);
Expand Down
4 changes: 4 additions & 0 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ typedef void (*OnOpenCommissioningWindow)(void * context, NodeId deviceId, CHIP_
* relevant information when the application tries to communicate with the device
*/
class DLL_EXPORT DeviceController : public SessionReleaseDelegate,
public SessionRecoveryDelegate,
#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
public AbstractDnssdDiscoveryController,
#endif
Expand Down Expand Up @@ -379,6 +380,9 @@ class DLL_EXPORT DeviceController : public SessionReleaseDelegate,
//////////// SessionReleaseDelegate Implementation ///////////////
void OnSessionReleased(SessionHandle session) override;

//////////// SessionRecoveryDelegate Implementation ///////////////
void OnFirstMessageDeliveryFailed(const SessionHandle & session) override;

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
//////////// ResolverDelegate Implementation ///////////////
void OnNodeIdResolved(const chip::Dnssd::ResolvedNodeData & nodeData) override;
Expand Down
5 changes: 4 additions & 1 deletion src/controller/tests/TestCommissionableNodeController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ class MockResolver : public Resolver
CHIP_ERROR Init(chip::Inet::InetLayer * inetLayer) override { return InitStatus; }
void Shutdown() override {}
void SetResolverDelegate(ResolverDelegate *) override {}
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override { return ResolveNodeIdStatus; }
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type, Resolver::CacheBypass dnssdCacheBypass) override
{
return ResolveNodeIdStatus;
}
CHIP_ERROR FindCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return FindCommissionersStatus; }
CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; }

Expand Down
9 changes: 9 additions & 0 deletions src/lib/core/CHIPConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -2702,6 +2702,15 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
#define CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES 2
#endif

/**
* @def CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES
*
* @brief Defines the max number of SessionRecoveryDelegate
*/
#ifndef CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES
#define CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES 3
#endif

/**
* @}
*/
16 changes: 10 additions & 6 deletions src/lib/dnssd/Discovery_ImplPlatform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -459,17 +459,21 @@ CHIP_ERROR DiscoveryImplPlatform::FinalizeServiceUpdate()
return ChipDnssdFinalizeServiceUpdate();
}

CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type)
CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type,
Resolver::CacheBypass dnssdCacheBypass)
{
ReturnErrorOnFailure(InitImpl());

#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0
/* see if the entry is cached and use it.... */
ResolvedNodeData nodeData;
if (sDnssdCache.Lookup(peerId, nodeData) == CHIP_NO_ERROR)
if (dnssdCacheBypass == Resolver::CacheBypass::Off)
{
mResolverDelegate->OnNodeIdResolved(nodeData);
return CHIP_NO_ERROR;
/* see if the entry is cached and use it.... */
ResolvedNodeData nodeData;
if (sDnssdCache.Lookup(peerId, nodeData) == CHIP_NO_ERROR)
{
mResolverDelegate->OnNodeIdResolved(nodeData);
return CHIP_NO_ERROR;
}
}
#endif

Expand Down
2 changes: 1 addition & 1 deletion src/lib/dnssd/Discovery_ImplPlatform.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver

// Members that implement Resolver interface.
void SetResolverDelegate(ResolverDelegate * delegate) override { mResolverDelegate = delegate; }
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type, Resolver::CacheBypass dnssdCacheBypass) override;
CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR FindCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;

Expand Down
12 changes: 11 additions & 1 deletion src/lib/dnssd/Resolver.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,12 @@ class ResolverDelegate
class Resolver
{
public:
enum class CacheBypass
{
On,
Off
};

virtual ~Resolver() {}

/**
Expand All @@ -276,11 +282,15 @@ class Resolver
/**
* Requests resolution of the given operational node service.
*
* If `dnssdCacheBypass` is set to `On` it forces resolution of the given node and bypass option
* of using DNS-SD cache.
*
* When the operation succeeds or fails, and a resolver delegate has been registered,
* the result of the operation is passed to the delegate's `OnNodeIdResolved` or
* `OnNodeIdResolutionFailed` method, respectively.
*/
virtual CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) = 0;
virtual CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type,
Resolver::CacheBypass dnssdCacheBypass = CacheBypass::Off) = 0;

/**
* Finds all commissionable nodes matching the given filter.
Expand Down
4 changes: 2 additions & 2 deletions src/lib/dnssd/Resolver_ImplMinimalMdns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate
CHIP_ERROR Init(chip::Inet::InetLayer * inetLayer) override;
void Shutdown() override;
void SetResolverDelegate(ResolverDelegate * delegate) override { mDelegate = delegate; }
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type, Resolver::CacheBypass dnssdCacheBypass) override;
CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR FindCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;

Expand Down Expand Up @@ -500,7 +500,7 @@ CHIP_ERROR MinMdnsResolver::BrowseNodes(DiscoveryType type, DiscoveryFilter filt
return SendQuery(qname, mdns::Minimal::QType::ANY);
}

CHIP_ERROR MinMdnsResolver::ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type)
CHIP_ERROR MinMdnsResolver::ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type, Resolver::CacheBypass dnssdCacheBypass)
{
mDiscoveryType = DiscoveryType::kOperational;
mActiveResolves.MarkPending(peerId);
Expand Down
2 changes: 1 addition & 1 deletion src/lib/dnssd/Resolver_ImplNone.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class NoneResolver : public Resolver
void Shutdown() override {}
void SetResolverDelegate(ResolverDelegate *) override {}

CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type, Resolver::CacheBypass dnssdCacheBypass) override
{
ChipLogError(Discovery, "Failed to resolve node ID: dnssd resolving not available");
return CHIP_ERROR_NOT_IMPLEMENTED;
Expand Down
2 changes: 1 addition & 1 deletion src/lib/shell/commands/Dns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ CHIP_ERROR ResolveHandler(int argc, char ** argv)
peerId.SetCompressedFabricId(strtoull(argv[0], NULL, 10));
peerId.SetNodeId(strtoull(argv[1], NULL, 10));

return Dnssd::Resolver::Instance().ResolveNodeId(peerId, Inet::IPAddressType::kAny);
return Dnssd::Resolver::Instance().ResolveNodeId(peerId, Inet::IPAddressType::kAny, Dnssd::Resolver::CacheBypass::On);
}

bool ParseSubType(int argc, char ** argv, Dnssd::DiscoveryFilter & filter)
Expand Down
12 changes: 12 additions & 0 deletions src/messaging/ReliableMessageMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <lib/support/logging/CHIPLogging.h>
#include <messaging/ErrorCategory.h>
#include <messaging/ExchangeMessageDispatch.h>
#include <messaging/ExchangeMgr.h>
#include <messaging/Flags.h>
#include <messaging/ReliableMessageContext.h>

Expand Down Expand Up @@ -353,6 +354,17 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry)

if (err == CHIP_NO_ERROR)
{
const ExchangeManager * exchangeMgr = entry->ec->GetExchangeMgr();
// TODO: investigate why in ReliableMessageMgr::CheckResendApplicationMessageWithPeerExchange unit test released exchange
// context with mExchangeMgr==nullptr is used.
if (exchangeMgr)
{
// After the first failure notify session manager to refresh device data
if (entry->sendCount == 0)
{
exchangeMgr->GetSessionManager()->RefreshSessionOperationalData(entry->ec->GetSessionHandle());
}
}
// Update the counters
entry->sendCount++;
}
Expand Down
15 changes: 15 additions & 0 deletions src/transport/SessionDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,19 @@ class DLL_EXPORT SessionReleaseDelegate
virtual void OnSessionReleased(SessionHandle session) = 0;
};

class DLL_EXPORT SessionRecoveryDelegate
{
public:
virtual ~SessionRecoveryDelegate() {}

/**
* @brief
* Called when the first message delivery in a session failed,
* so actions aiming to recover connection can be performed.
*
* @param session The handle to the secure session
*/
virtual void OnFirstMessageDeliveryFailed(const SessionHandle & session) = 0;
};

} // namespace chip
32 changes: 32 additions & 0 deletions src/transport/SessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,38 @@ void SessionManager::OnMessageReceived(const PeerAddress & peerAddress, System::
}
}

void SessionManager::RegisterRecoveryDelegate(SessionRecoveryDelegate & cb)
{
#ifndef NDEBUG
mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper<SessionRecoveryDelegate> * i) {
VerifyOrDie(std::addressof(cb) != std::addressof(i->get()));
return true;
});
#endif
std::reference_wrapper<SessionRecoveryDelegate> * slot = mSessionRecoveryDelegates.CreateObject(cb);
VerifyOrDie(slot != nullptr);
}

void SessionManager::UnregisterRecoveryDelegate(SessionRecoveryDelegate & cb)
{
mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper<SessionRecoveryDelegate> * i) {
if (std::addressof(cb) == std::addressof(i->get()))
{
mSessionRecoveryDelegates.ReleaseObject(i);
return false;
}
return true;
});
}

void SessionManager::RefreshSessionOperationalData(const SessionHandle & sessionHandle)
{
mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper<SessionRecoveryDelegate> * cb) {
cb->get().OnFirstMessageDeliveryFailed(sessionHandle);
return true;
});
}

void SessionManager::MessageDispatch(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress,
System::PacketBufferHandle && msg)
{
Expand Down
7 changes: 7 additions & 0 deletions src/transport/SessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,10 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
});
}

void RegisterRecoveryDelegate(SessionRecoveryDelegate & cb);
void UnregisterRecoveryDelegate(SessionRecoveryDelegate & cb);
void RefreshSessionOperationalData(const SessionHandle & sessionHandle);

/**
* @brief
* Establish a new pairing with a peer node
Expand Down Expand Up @@ -291,6 +295,9 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
BitMapObjectPool<std::reference_wrapper<SessionReleaseDelegate>, CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES>
mSessionReleaseDelegates;

BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
mSessionRecoveryDelegates;

TransportMgrBase * mTransportMgr = nullptr;
Transport::MessageCounterManagerInterface * mMessageCounterManager = nullptr;

Expand Down

0 comments on commit 3958429

Please sign in to comment.