From 14c53de40d2af2705dca8138b1ad41352147f531 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Wed, 24 Nov 2021 10:37:52 +0100 Subject: [PATCH] Added automatic node resolve after first transmission failure. * 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. --- .../chip-tool/commands/discover/Commands.h | 3 +- src/app/CASESessionManager.cpp | 3 +- src/controller/CHIPDeviceController.cpp | 8 +++++ src/controller/CHIPDeviceController.h | 4 +++ .../TestCommissionableNodeController.cpp | 5 ++- src/lib/core/CHIPConfig.h | 9 ++++++ src/lib/dnssd/Discovery_ImplPlatform.cpp | 16 ++++++---- src/lib/dnssd/Discovery_ImplPlatform.h | 2 +- src/lib/dnssd/Resolver.h | 12 ++++++- src/lib/dnssd/Resolver_ImplMinimalMdns.cpp | 4 +-- src/lib/dnssd/Resolver_ImplNone.cpp | 2 +- src/lib/shell/commands/Dns.cpp | 2 +- src/messaging/ReliableMessageMgr.cpp | 12 +++++++ src/transport/SessionDelegate.h | 15 +++++++++ src/transport/SessionManager.cpp | 32 +++++++++++++++++++ src/transport/SessionManager.h | 7 ++++ 16 files changed, 121 insertions(+), 15 deletions(-) diff --git a/examples/chip-tool/commands/discover/Commands.h b/examples/chip-tool/commands/discover/Commands.h index 5f74b42ca82037..e09a837d6f1810 100644 --- a/examples/chip-tool/commands/discover/Commands.h +++ b/examples/chip-tool/commands/discover/Commands.h @@ -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 diff --git a/src/app/CASESessionManager.cpp b/src/app/CASESessionManager.cpp index b14542272e3414..f5c76239b2a04c 100644 --- a/src/app/CASESessionManager.cpp +++ b/src/app/CASESessionManager.cpp @@ -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) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index e1be7c1d92116e..a0ff7bc14c3f58 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -268,6 +268,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; @@ -542,6 +549,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); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 7cfced16dcce6b..244c002cbf396a 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -187,6 +187,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 @@ -378,6 +379,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; diff --git a/src/controller/tests/TestCommissionableNodeController.cpp b/src/controller/tests/TestCommissionableNodeController.cpp index 1a9fbd8f7dbac8..c1101eca7ebb97 100644 --- a/src/controller/tests/TestCommissionableNodeController.cpp +++ b/src/controller/tests/TestCommissionableNodeController.cpp @@ -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; } diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index bebb3037afd0c8..0c35f91e15bbee 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -2690,6 +2690,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 + /** * @} */ diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index c6f55c85dc6777..ea94fed4763e47 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -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 diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h index 076052ab042bc2..5ac1753493d6ca 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.h +++ b/src/lib/dnssd/Discovery_ImplPlatform.h @@ -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; diff --git a/src/lib/dnssd/Resolver.h b/src/lib/dnssd/Resolver.h index d44c55b9a6fc1b..6512ada84f34f0 100644 --- a/src/lib/dnssd/Resolver.h +++ b/src/lib/dnssd/Resolver.h @@ -252,6 +252,12 @@ class ResolverDelegate class Resolver { public: + enum class CacheBypass + { + On, + Off + }; + virtual ~Resolver() {} /** @@ -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. diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp index 739894d3a55e5f..da09d23722fe82 100644 --- a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp @@ -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; @@ -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); diff --git a/src/lib/dnssd/Resolver_ImplNone.cpp b/src/lib/dnssd/Resolver_ImplNone.cpp index fc1f0870859ae2..c3c28ac8d138b2 100644 --- a/src/lib/dnssd/Resolver_ImplNone.cpp +++ b/src/lib/dnssd/Resolver_ImplNone.cpp @@ -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; diff --git a/src/lib/shell/commands/Dns.cpp b/src/lib/shell/commands/Dns.cpp index 5512b5258f76be..8f8bb31ffeb50d 100644 --- a/src/lib/shell/commands/Dns.cpp +++ b/src/lib/shell/commands/Dns.cpp @@ -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) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index a156f7d489b07b..8594d080bdadda 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -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++; } diff --git a/src/transport/SessionDelegate.h b/src/transport/SessionDelegate.h index 893b063071ba53..aacb6e7824245b 100644 --- a/src/transport/SessionDelegate.h +++ b/src/transport/SessionDelegate.h @@ -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 diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index 8d6bc02a86352a..e32eb3e5678b2b 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -358,6 +358,38 @@ void SessionManager::OnMessageReceived(const PeerAddress & peerAddress, System:: } } +void SessionManager::RegisterRecoveryDelegate(SessionRecoveryDelegate & cb) +{ +#ifndef NDEBUG + mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper * i) { + VerifyOrDie(std::addressof(cb) != std::addressof(i->get())); + return true; + }); +#endif + std::reference_wrapper * slot = mSessionRecoveryDelegates.CreateObject(cb); + VerifyOrDie(slot != nullptr); +} + +void SessionManager::UnregisterRecoveryDelegate(SessionRecoveryDelegate & cb) +{ + mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper * 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 * cb) { + cb->get().OnFirstMessageDeliveryFailed(sessionHandle); + return true; + }); +} + void SessionManager::MessageDispatch(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress, System::PacketBufferHandle && msg) { diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 7112542593c784..3cd472ff49f6be 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -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 @@ -291,6 +295,9 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate BitMapObjectPool, CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES> mSessionReleaseDelegates; + BitMapObjectPool, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES> + mSessionRecoveryDelegates; + TransportMgrBase * mTransportMgr = nullptr; Transport::MessageCounterManagerInterface * mMessageCounterManager = nullptr;