From 3d1a35c4c28c0e669969fb6ae94eda7e2eba89df Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 9 Dec 2022 15:46:12 -0500 Subject: [PATCH 1/3] Add a way for Resolver consumers to cancel operational resolve attempts. Adds a way for consumers to notify Resolver when they no longer care about an operational resolve, so a Resolver implementation can keep track of how many consumers are interested and stop work as desired if no one is interested. Fixes https://github.com/project-chip/connectedhomeip/issues/23881 --- .../python/chip/discovery/NodeResolution.cpp | 2 + .../TestCommissionableNodeController.cpp | 1 + .../AddressResolve_DefaultImpl.cpp | 7 +++ src/lib/dnssd/ActiveResolveAttempts.cpp | 17 +++++ src/lib/dnssd/ActiveResolveAttempts.h | 33 +++++++++- src/lib/dnssd/Discovery_ImplPlatform.cpp | 30 ++++++++- src/lib/dnssd/Discovery_ImplPlatform.h | 1 + src/lib/dnssd/Resolver.h | 24 ++++++- src/lib/dnssd/ResolverProxy.h | 1 + src/lib/dnssd/Resolver_ImplMinimalMdns.cpp | 11 ++++ src/lib/dnssd/Resolver_ImplNone.cpp | 6 ++ src/lib/dnssd/platform/Dnssd.h | 7 +++ src/lib/shell/commands/Dns.cpp | 8 ++- src/platform/Ameba/DnssdImpl.cpp | 2 + src/platform/Darwin/DnssdContexts.cpp | 63 +++++++++++++++---- src/platform/Darwin/DnssdImpl.cpp | 53 +++++++++++++++- src/platform/Darwin/DnssdImpl.h | 19 +++++- src/platform/ESP32/DnssdImpl.cpp | 2 + src/platform/Linux/DnssdImpl.cpp | 2 + src/platform/OpenThread/DnssdImpl.cpp | 1 + src/platform/Tizen/DnssdImpl.cpp | 2 + src/platform/android/DnssdImpl.cpp | 2 + src/platform/bouffalolab/BL602/DnssdImpl.cpp | 2 + src/platform/fake/DnssdImpl.cpp | 2 + src/platform/mt793x/DnssdImpl.cpp | 2 + src/platform/webos/DnssdImpl.cpp | 2 + 26 files changed, 278 insertions(+), 24 deletions(-) diff --git a/src/controller/python/chip/discovery/NodeResolution.cpp b/src/controller/python/chip/discovery/NodeResolution.cpp index 153cc1ef438643..96afefe1943397 100644 --- a/src/controller/python/chip/discovery/NodeResolution.cpp +++ b/src/controller/python/chip/discovery/NodeResolution.cpp @@ -37,6 +37,7 @@ class PythonResolverDelegate : public OperationalResolveDelegate public: void OnOperationalNodeResolved(const ResolvedNodeData & nodeData) override { + Resolver::Instance().NodeIdResolutionNoLongerNeeded(nodeData.operationalData.peerId); if (mSuccessCallback != nullptr) { char ipAddressBuffer[128]; @@ -59,6 +60,7 @@ class PythonResolverDelegate : public OperationalResolveDelegate void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override { + Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId); if (mFailureCallback != nullptr) { mFailureCallback(peerId.GetCompressedFabricId(), peerId.GetNodeId(), ToPyChipError(error)); diff --git a/src/controller/tests/TestCommissionableNodeController.cpp b/src/controller/tests/TestCommissionableNodeController.cpp index 29893620c971f0..6d0c04ea1f831b 100644 --- a/src/controller/tests/TestCommissionableNodeController.cpp +++ b/src/controller/tests/TestCommissionableNodeController.cpp @@ -37,6 +37,7 @@ class MockResolver : public Resolver void SetOperationalDelegate(OperationalResolveDelegate * delegate) override {} void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override {} CHIP_ERROR ResolveNodeId(const PeerId & peerId) override { return ResolveNodeIdStatus; } + void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override {} CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return DiscoverCommissionersStatus; } CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override { diff --git a/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp b/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp index 17b6f623f43c9f..c687f0121b4641 100644 --- a/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp +++ b/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp @@ -127,6 +127,7 @@ CHIP_ERROR Resolver::CancelLookup(Impl::NodeLookupHandle & handle, FailureCallba { VerifyOrReturnError(handle.IsActive(), CHIP_ERROR_INVALID_ARGUMENT); mActiveLookups.Remove(&handle); + Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(handle.GetRequest().GetPeerId()); // Adjust any timing updates. ReArmTimer(); @@ -164,6 +165,7 @@ void Resolver::Shutdown() mActiveLookups.Erase(current); + Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId); // Failure callback only called after iterator was cleared: // This allows failure handlers to deallocate structures that may // contain the active lookup data as a member (intrusive lists members) @@ -231,6 +233,8 @@ void Resolver::HandleAction(IntrusiveList::Iterator & current) NodeListener * listener = current->GetListener(); mActiveLookups.Erase(current); + Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId); + // ensure action is taken AFTER the current current lookup is marked complete // This allows failure handlers to deallocate structures that may // contain the active lookup data as a member (intrusive lists members) @@ -277,6 +281,8 @@ void Resolver::OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERR NodeListener * listener = current->GetListener(); mActiveLookups.Erase(current); + Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId); + // Failure callback only called after iterator was cleared: // This allows failure handlers to deallocate structures that may // contain the active lookup data as a member (intrusive lists members) @@ -326,6 +332,7 @@ void Resolver::ReArmTimer() mActiveLookups.Erase(it); it = mActiveLookups.begin(); + Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId); // Callback only called after active lookup is cleared // This allows failure handlers to deallocate structures that may // contain the active lookup data as a member (intrusive lists members) diff --git a/src/lib/dnssd/ActiveResolveAttempts.cpp b/src/lib/dnssd/ActiveResolveAttempts.cpp index 9e458207aeb3f7..1f41a58414d268 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.cpp +++ b/src/lib/dnssd/ActiveResolveAttempts.cpp @@ -92,6 +92,22 @@ CHIP_ERROR ActiveResolveAttempts::CompleteAllBrowses() return CHIP_NO_ERROR; } +void ActiveResolveAttempts::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) +{ + for (auto & item : mRetryQueue) + { + if (item.attempt.Matches(peerId)) + { + item.attempt.ConsumerRemoved(); + if (item.attempt.ResolveData().consumerCount == 0) + { + item.attempt.Clear(); + } + return; + } + } +} + void ActiveResolveAttempts::MarkPending(const chip::PeerId & peerId) { MarkPending(ScheduledAttempt(peerId, /* firstSend */ true)); @@ -172,6 +188,7 @@ void ActiveResolveAttempts::MarkPending(ScheduledAttempt && attempt) ChipLogError(Discovery, "Re-using pending resolve entry before reply was received."); } + attempt.WillCoalesceWith(entryToUse->attempt); entryToUse->attempt = attempt; entryToUse->queryDueTime = mClock->GetMonotonicTimestamp(); entryToUse->nextRetryDelay = System::Clock::Seconds16(1); diff --git a/src/lib/dnssd/ActiveResolveAttempts.h b/src/lib/dnssd/ActiveResolveAttempts.h index 6ba51cb151e7b5..d3b0e6b3b4900f 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.h +++ b/src/lib/dnssd/ActiveResolveAttempts.h @@ -58,6 +58,7 @@ class ActiveResolveAttempts struct Resolve { chip::PeerId peerId; + uint32_t consumerCount = 0; Resolve(chip::PeerId id) : peerId(id) {} }; @@ -68,7 +69,11 @@ class ActiveResolveAttempts IpResolve(HeapQName && host) : hostName(std::move(host)) {} }; - ScheduledAttempt() {} + ScheduledAttempt() + { + static_assert(sizeof(Resolve) <= sizeof(Browse) || sizeof(Resolve) <= sizeof(HeapQName), + "Figure out where to put the Resolve counter."); + } ScheduledAttempt(const chip::PeerId & peer, bool first) : resolveData(chip::InPlaceTemplateType(), peer), firstSend(first) {} @@ -181,6 +186,28 @@ class ActiveResolveAttempts bool IsIpResolve() const { return resolveData.Is(); } void Clear() { resolveData = DataType(); } + void WillCoalesceWith(const ScheduledAttempt & existing) + { + if (IsResolve() && existing.Matches(*this)) + { + resolveData.Get().consumerCount = existing.resolveData.Get().consumerCount + 1; + } + } + + void ConsumerRemoved() + { + if (!IsResolve()) + { + return; + } + + auto & count = resolveData.Get().consumerCount; + if (count > 0) + { + --count; + } + } + const Browse & BrowseData() const { return resolveData.Get(); } const Resolve & ResolveData() const { return resolveData.Get(); } const IpResolve & IpResolveData() const { return resolveData.Get(); } @@ -208,6 +235,10 @@ class ActiveResolveAttempts /// from the internal list. CHIP_ERROR CompleteAllBrowses(); + /// Note that resolve attempts for the given peer id now have one fewer + /// consumer. + void NodeIdResolutionNoLongerNeeded(const chip::PeerId & peerId); + /// Mark that a resolution is pending, adding it to the internal list /// /// Once this complete, this peer id will be returned immediately diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index 8a6f1976c557ca..32c6d4ad87df5f 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -625,6 +625,15 @@ CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId) return mResolverProxy.ResolveNodeId(peerId); } +void DiscoveryImplPlatform::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) +{ + char name[Common::kInstanceNameMaxLength + 1]; + if (MakeInstanceName(name, sizeof(name), peerId) == CHIP_NO_ERROR) + { + ChipDnssdResolveNoLongerNeeded(name); + } +} + CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissionableNodes(DiscoveryFilter filter) { ReturnErrorOnFailure(InitImpl()); @@ -671,15 +680,30 @@ CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId) ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...", ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId())); - mDelegate->Retain(); - DnssdService service; ReturnErrorOnFailure(MakeInstanceName(service.mName, sizeof(service.mName), peerId)); Platform::CopyString(service.mType, kOperationalServiceName); service.mProtocol = DnssdServiceProtocol::kDnssdProtocolTcp; service.mAddressType = Inet::IPAddressType::kAny; - return ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, mDelegate); + + mDelegate->Retain(); + + CHIP_ERROR err = ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, mDelegate); + if (err != CHIP_NO_ERROR) + { + mDelegate->Release(); + } + return err; +} + +void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) +{ + char name[Common::kInstanceNameMaxLength + 1]; + if (MakeInstanceName(name, sizeof(name), peerId) == CHIP_NO_ERROR) + { + ChipDnssdResolveNoLongerNeeded(name); + } } ResolverProxy::~ResolverProxy() diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h index bf044ecb962602..36c191d714bcc7 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.h +++ b/src/lib/dnssd/Discovery_ImplPlatform.h @@ -55,6 +55,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver mResolverProxy.SetCommissioningDelegate(delegate); } CHIP_ERROR ResolveNodeId(const PeerId & peerId) override; + void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override; CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override; CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override; CHIP_ERROR StopDiscovery() override; diff --git a/src/lib/dnssd/Resolver.h b/src/lib/dnssd/Resolver.h index 115e7e219231ab..094459c9b4c27d 100644 --- a/src/lib/dnssd/Resolver.h +++ b/src/lib/dnssd/Resolver.h @@ -369,11 +369,31 @@ class Resolver * This will trigger a DNSSD query. * * 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. + * the result of the operation is passed to the delegate's `OnOperationalNodeResolved` or + * `OnOperationalNodeResolutionFailed` method, respectively. + * + * Multiple calls to ResolveNodeId may be coalesced by the implementation + * and lead to just one call to + * OnOperationalNodeResolved/OnOperationalNodeResolutionFailed, as long as + * the later calls cause the underlying querying mechanism to re-query as if + * there were no coalescing. + * + * A single call to ResolveNodeId may lead to multiple calls to + * OnOperationalNodeResolved with different IP addresses. + * + * @see NodeIdResolutionNoLongerNeeded. */ virtual CHIP_ERROR ResolveNodeId(const PeerId & peerId) = 0; + /* + * Notify the resolver that one of the consumers that called ResolveNodeId + * successfully no longer needs the resolution result (e.g. because it got + * the result, or got an error, or no longer cares about future updates). + * There must be a NodeIdResolutionNoLongerNeeded call that matches every + * successful ResolveNodeId call. + */ + virtual void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) = 0; + /** * Finds all commissionable nodes matching the given filter. * diff --git a/src/lib/dnssd/ResolverProxy.h b/src/lib/dnssd/ResolverProxy.h index 5b16cd2def1513..58025bdd14ea72 100644 --- a/src/lib/dnssd/ResolverProxy.h +++ b/src/lib/dnssd/ResolverProxy.h @@ -150,6 +150,7 @@ class ResolverProxy : public Resolver } CHIP_ERROR ResolveNodeId(const PeerId & peerId) override; + void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override; CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override; CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override; CHIP_ERROR StopDiscovery() override; diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp index 5534ab1cb64b11..5faacb025d9348 100644 --- a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp @@ -279,6 +279,7 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mOperationalDelegate = delegate; } void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override { mCommissioningDelegate = delegate; } CHIP_ERROR ResolveNodeId(const PeerId & peerId) override; + void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override; CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override; CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override; CHIP_ERROR StopDiscovery() override; @@ -659,6 +660,11 @@ CHIP_ERROR MinMdnsResolver::ResolveNodeId(const PeerId & peerId) return SendAllPendingQueries(); } +void MinMdnsResolver::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) +{ + mActiveResolves.NodeIdResolutionNoLongerNeeded(peerId); +} + CHIP_ERROR MinMdnsResolver::ScheduleRetries() { ReturnErrorCodeIf(mSystemLayer == nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -710,6 +716,11 @@ CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId) return chip::Dnssd::Resolver::Instance().ResolveNodeId(peerId); } +void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) +{ + return chip::Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId); +} + CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) { VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); diff --git a/src/lib/dnssd/Resolver_ImplNone.cpp b/src/lib/dnssd/Resolver_ImplNone.cpp index 89fce74c065548..ce4f0072efd3f9 100644 --- a/src/lib/dnssd/Resolver_ImplNone.cpp +++ b/src/lib/dnssd/Resolver_ImplNone.cpp @@ -37,6 +37,10 @@ class NoneResolver : public Resolver ChipLogError(Discovery, "Failed to resolve node ID: dnssd resolving not available"); return CHIP_ERROR_NOT_IMPLEMENTED; } + void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override + { + ChipLogError(Discovery, "Failed to stop resolving node ID: dnssd resolving not available"); + } CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; @@ -68,6 +72,8 @@ CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId) return CHIP_ERROR_NOT_IMPLEMENTED; } +void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) {} + CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/lib/dnssd/platform/Dnssd.h b/src/lib/dnssd/platform/Dnssd.h index 188ff52f980ee9..7c21f9e1012952 100644 --- a/src/lib/dnssd/platform/Dnssd.h +++ b/src/lib/dnssd/platform/Dnssd.h @@ -245,6 +245,13 @@ CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier); CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, chip::Inet::InterfaceId interface, DnssdResolveCallback callback, void * context); +/** + * This function notifies the implementation that a resolve result is no longer + * needed by some consumer, to allow implementations to stop unnecessary resolve + * work. + */ +void ChipDnssdResolveNoLongerNeeded(const char * instanceName); + /** * This function asks the mdns daemon to asynchronously reconfirm an address that appears to be out of date. * diff --git a/src/lib/shell/commands/Dns.cpp b/src/lib/shell/commands/Dns.cpp index 305e142050295e..0717d4235f4d9b 100644 --- a/src/lib/shell/commands/Dns.cpp +++ b/src/lib/shell/commands/Dns.cpp @@ -37,12 +37,14 @@ namespace { Shell::Engine sShellDnsBrowseSubcommands; Shell::Engine sShellDnsSubcommands; +Dnssd::ResolverProxy sResolverProxy; class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, public Dnssd::CommissioningResolveDelegate { public: void OnOperationalNodeResolved(const Dnssd::ResolvedNodeData & nodeData) override { + sResolverProxy.NodeIdResolutionNoLongerNeeded(nodeData.operationalData.peerId); streamer_printf(streamer_get(), "DNS resolve for " ChipLogFormatX64 "-" ChipLogFormatX64 " succeeded:\r\n", ChipLogValueX64(nodeData.operationalData.peerId.GetCompressedFabricId()), ChipLogValueX64(nodeData.operationalData.peerId.GetNodeId())); @@ -66,7 +68,10 @@ class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, publi streamer_printf(streamer_get(), " Supports TCP: %s\r\n", nodeData.resolutionData.supportsTcp ? "yes" : "no"); } - void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override {} + void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override + { + sResolverProxy.NodeIdResolutionNoLongerNeeded(peerId); + } void OnNodeDiscovered(const Dnssd::DiscoveredNodeData & nodeData) override { @@ -116,7 +121,6 @@ class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, publi }; DnsShellResolverDelegate sDnsShellResolverDelegate; -Dnssd::ResolverProxy sResolverProxy; CHIP_ERROR ResolveHandler(int argc, char ** argv) { diff --git a/src/platform/Ameba/DnssdImpl.cpp b/src/platform/Ameba/DnssdImpl.cpp index d12f006e5834b6..f75223ec10cb81 100644 --- a/src/platform/Ameba/DnssdImpl.cpp +++ b/src/platform/Ameba/DnssdImpl.cpp @@ -114,5 +114,7 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * /*service*/, chip::Inet::InterfaceId return CHIP_ERROR_NOT_IMPLEMENTED; } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + } // namespace Dnssd } // namespace chip diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index 2f2ae01e5e6bd8..b433dc47a418c7 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -166,10 +166,8 @@ CHIP_ERROR MdnsContexts::Add(GenericContext * context, DNSServiceRef sdRef) return CHIP_NO_ERROR; } -CHIP_ERROR MdnsContexts::Remove(GenericContext * context) +bool MdnsContexts::RemoveWithoutDeleting(GenericContext * context) { - bool found = false; - std::vector::const_iterator iter = mContexts.cbegin(); while (iter != mContexts.cend()) { @@ -179,12 +177,20 @@ CHIP_ERROR MdnsContexts::Remove(GenericContext * context) continue; } - Delete(*iter); mContexts.erase(iter); - found = true; - break; + return true; } + return false; +} + +CHIP_ERROR MdnsContexts::Remove(GenericContext * context) +{ + bool found = RemoveWithoutDeleting(context); + if (found) + { + Delete(context); + } return found ? CHIP_NO_ERROR : CHIP_ERROR_KEY_NOT_FOUND; } @@ -251,6 +257,19 @@ CHIP_ERROR MdnsContexts::GetRegisterContextOfType(const char * type, RegisterCon return found ? CHIP_NO_ERROR : CHIP_ERROR_KEY_NOT_FOUND; } +ResolveContext * MdnsContexts::GetExistingResolveForInstanceName(const char * instanceName) +{ + for (auto & ctx : mContexts) + { + if (ctx->type == ContextType::Resolve && (static_cast(ctx))->Matches(instanceName)) + { + return static_cast(ctx); + } + } + + return nullptr; +} + RegisterContext::RegisterContext(const char * sType, const char * instanceName, DnssdPublishCallback cb, void * cbContext) { type = ContextType::Register; @@ -305,12 +324,15 @@ void BrowseContext::DispatchPartialSuccess() services.clear(); } -ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType) +ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType, + const char * instanceNameToResolve, std::shared_ptr && consumerCounterToUse) { - type = ContextType::Resolve; - context = cbContext; - callback = cb; - protocol = GetProtocol(cbAddressType); + type = ContextType::Resolve; + context = cbContext; + callback = cb; + protocol = GetProtocol(cbAddressType); + instanceName = instanceNameToResolve; + consumerCounter = std::move(consumerCounterToUse); } ResolveContext::~ResolveContext() {} @@ -318,12 +340,24 @@ ResolveContext::~ResolveContext() {} void ResolveContext::DispatchFailure(DNSServiceErrorType err) { ChipLogError(Discovery, "Mdns: Resolve failure (%s)", Error::ToString(err)); + // Remove before dispatching, so calls back into + // ChipDnssdResolveNoLongerNeeded don't find us and try to also remove us. + bool needDelete = MdnsContexts::GetInstance().RemoveWithoutDeleting(this); + callback(context, nullptr, Span(), Error::ToChipError(err)); - MdnsContexts::GetInstance().Remove(this); + + if (needDelete) + { + MdnsContexts::GetInstance().Delete(this); + } } void ResolveContext::DispatchSuccess() { + // Remove before dispatching, so calls back into + // ChipDnssdResolveNoLongerNeeded don't find us and try to also remove us. + bool needDelete = MdnsContexts::GetInstance().RemoveWithoutDeleting(this); + for (auto & interface : interfaces) { auto & ips = interface.second.addresses; @@ -339,7 +373,10 @@ void ResolveContext::DispatchSuccess() break; } - MdnsContexts::GetInstance().Remove(this); + if (needDelete) + { + MdnsContexts::GetInstance().Delete(this); + } } CHIP_ERROR ResolveContext::OnNewAddress(uint32_t interfaceId, const struct sockaddr * address) diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index 2718cf4abaf25e..b4100271e7eb8d 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -344,7 +344,23 @@ static CHIP_ERROR Resolve(void * context, DnssdResolveCallback callback, uint32_ ChipLogDetail(Discovery, "Resolve type=%s name=%s interface=%" PRIu32, StringOrNullMarker(type), StringOrNullMarker(name), interfaceId); - auto sdCtx = chip::Platform::New(context, callback, addressType); + // This is a little silly, in that resolves for the sane name, type, etc get + // coalesced by the underlying mDNSResponder anyway. But we need to keep + // track of our context/callback/etc, (even though in practice it's always + // exactly the same) and the interface id (which might actually be different + // for different Resolve calls). So for now just keep using a + // ResolveContext to track all that. + std::shared_ptr counterHolder; + if (auto existingCtx = MdnsContexts::GetInstance().GetExistingResolveForInstanceName(name)) + { + counterHolder = existingCtx->consumerCounter; + } + else + { + counterHolder = std::make_shared(0); + } + + auto sdCtx = chip::Platform::New(context, callback, addressType, name, std::move(counterHolder)); VerifyOrReturnError(nullptr != sdCtx, CHIP_ERROR_NO_MEMORY); auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); @@ -354,7 +370,12 @@ static CHIP_ERROR Resolve(void * context, DnssdResolveCallback callback, uint32_ err = DNSServiceResolve(&sdRefCopy, kResolveFlags, interfaceId, name, type, kLocalDot, OnResolve, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - return MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef); + CHIP_ERROR retval = MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef); + if (retval == CHIP_NO_ERROR) + { + (*(sdCtx->consumerCounter))++; + } + return retval; } } // namespace @@ -450,6 +471,34 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * service, chip::Inet::InterfaceId inte return Resolve(context, callback, interfaceId, service->mAddressType, regtype.c_str(), service->mName); } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) +{ + auto existingCtx = MdnsContexts::GetInstance().GetExistingResolveForInstanceName(instanceName); + if (!existingCtx) + { + return; + } + + if (*existingCtx->consumerCounter == 0) + { + return; + } + + (*existingCtx->consumerCounter)--; + + if (*existingCtx->consumerCounter == 0) + { + // No more consumers; clear out all of these resolves so they don't + // stick around. Dispatch timeout failure on all of them to make sure + // whatever kicked them off cleans up resources as needed. + do + { + existingCtx->Finalize(kDNSServiceErr_Timeout); + existingCtx = MdnsContexts::GetInstance().GetExistingResolveForInstanceName(instanceName); + } while (existingCtx != nullptr); + } +} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { VerifyOrReturnError(hostname != nullptr, CHIP_ERROR_INVALID_ARGUMENT); diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index 2c4e5c84a2618e..ca0e2362071821 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -50,6 +50,7 @@ struct GenericContext }; struct RegisterContext; +struct ResolveContext; class MdnsContexts { @@ -81,6 +82,18 @@ class MdnsContexts */ CHIP_ERROR GetRegisterContextOfType(const char * type, RegisterContext ** context); + /** + * Return a pointer to an existing ResolveContext for the given + * instanceName, if any. Returns nullptr if there are none. + */ + ResolveContext * GetExistingResolveForInstanceName(const char * instanceName); + + /** + * Remove context from the list, if it's present in the list. Return + * whether it was present. + */ + bool RemoveWithoutDeleting(GenericContext * context); + void Delete(GenericContext * context); private: @@ -141,8 +154,11 @@ struct ResolveContext : public GenericContext DnssdResolveCallback callback; std::map interfaces; DNSServiceProtocol protocol; + std::string instanceName; + std::shared_ptr consumerCounter; - ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType); + ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType, + const char * instanceNameToResolve, std::shared_ptr && consumerCounterToUse); virtual ~ResolveContext(); void DispatchFailure(DNSServiceErrorType err) override; @@ -155,6 +171,7 @@ struct ResolveContext : public GenericContext void OnNewInterface(uint32_t interfaceId, const char * fullname, const char * hostname, uint16_t port, uint16_t txtLen, const unsigned char * txtRecord); bool HasInterface(); + bool Matches(const char * otherInstanceName) const { return instanceName == otherInstanceName; } }; } // namespace Dnssd diff --git a/src/platform/ESP32/DnssdImpl.cpp b/src/platform/ESP32/DnssdImpl.cpp index 4f592b8666885b..2b31b1e6d7659b 100644 --- a/src/platform/ESP32/DnssdImpl.cpp +++ b/src/platform/ESP32/DnssdImpl.cpp @@ -511,6 +511,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * service, chip::Inet::InterfaceId inte return error; } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/Linux/DnssdImpl.cpp b/src/platform/Linux/DnssdImpl.cpp index 29665881efdcee..5cbe2fc07fb79d 100644 --- a/src/platform/Linux/DnssdImpl.cpp +++ b/src/platform/Linux/DnssdImpl.cpp @@ -872,6 +872,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, chip::Inet::InterfaceId browseResult->mAddressType, Inet::IPAddressType::kAny, interface, callback, context); } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/OpenThread/DnssdImpl.cpp b/src/platform/OpenThread/DnssdImpl.cpp index c85fba514f085a..5fbe3acba11537 100644 --- a/src/platform/OpenThread/DnssdImpl.cpp +++ b/src/platform/OpenThread/DnssdImpl.cpp @@ -122,6 +122,7 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, Inet::InterfaceId inter #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT && CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/Tizen/DnssdImpl.cpp b/src/platform/Tizen/DnssdImpl.cpp index 2bd1ed282863da..12a1da134ba70c 100644 --- a/src/platform/Tizen/DnssdImpl.cpp +++ b/src/platform/Tizen/DnssdImpl.cpp @@ -820,6 +820,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, chip::Inet::InterfaceId return DnssdTizen::GetInstance().Resolve(*browseResult, interface, callback, context); } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/android/DnssdImpl.cpp b/src/platform/android/DnssdImpl.cpp index 482db002dbd9c0..d0caa2ba3d741d 100644 --- a/src/platform/android/DnssdImpl.cpp +++ b/src/platform/android/DnssdImpl.cpp @@ -241,6 +241,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * service, Inet::InterfaceId interface, return CHIP_NO_ERROR; } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/bouffalolab/BL602/DnssdImpl.cpp b/src/platform/bouffalolab/BL602/DnssdImpl.cpp index e2e5779d0c5bf7..681f66aaa33657 100644 --- a/src/platform/bouffalolab/BL602/DnssdImpl.cpp +++ b/src/platform/bouffalolab/BL602/DnssdImpl.cpp @@ -313,6 +313,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * /*service*/, chip::Inet::InterfaceId return CHIP_ERROR_NOT_IMPLEMENTED; } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/fake/DnssdImpl.cpp b/src/platform/fake/DnssdImpl.cpp index fbce5984f7f059..618e96b71b4d94 100644 --- a/src/platform/fake/DnssdImpl.cpp +++ b/src/platform/fake/DnssdImpl.cpp @@ -132,6 +132,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, chip::Inet::InterfaceId return CHIP_ERROR_NOT_IMPLEMENTED; } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/mt793x/DnssdImpl.cpp b/src/platform/mt793x/DnssdImpl.cpp index b1c8748437892f..6d99d7f7d8d8ac 100644 --- a/src/platform/mt793x/DnssdImpl.cpp +++ b/src/platform/mt793x/DnssdImpl.cpp @@ -540,6 +540,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * service, chip::Inet::InterfaceId inte return error; } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/platform/webos/DnssdImpl.cpp b/src/platform/webos/DnssdImpl.cpp index 6d5f4a2f15e53e..1f75db6a47a08c 100644 --- a/src/platform/webos/DnssdImpl.cpp +++ b/src/platform/webos/DnssdImpl.cpp @@ -869,6 +869,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, chip::Inet::InterfaceId browseResult->mAddressType, Inet::IPAddressType::kAny, interface, callback, context); } +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} + CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { return CHIP_ERROR_NOT_IMPLEMENTED; From 4c18c092cb41416c30098dc62cfff8cd41a6a913 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 12 Dec 2022 10:09:42 -0500 Subject: [PATCH 2/3] Address review comments. --- src/lib/dnssd/Discovery_ImplPlatform.cpp | 14 ++++++-------- src/platform/Darwin/DnssdImpl.cpp | 15 ++++----------- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index 32c6d4ad87df5f..aab39d78e524a4 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -628,10 +628,9 @@ CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId) void DiscoveryImplPlatform::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) { char name[Common::kInstanceNameMaxLength + 1]; - if (MakeInstanceName(name, sizeof(name), peerId) == CHIP_NO_ERROR) - { - ChipDnssdResolveNoLongerNeeded(name); - } + ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId)); + + ChipDnssdResolveNoLongerNeeded(name); } CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissionableNodes(DiscoveryFilter filter) @@ -700,10 +699,9 @@ CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId) void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) { char name[Common::kInstanceNameMaxLength + 1]; - if (MakeInstanceName(name, sizeof(name), peerId) == CHIP_NO_ERROR) - { - ChipDnssdResolveNoLongerNeeded(name); - } + ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId)); + + ChipDnssdResolveNoLongerNeeded(name); } ResolverProxy::~ResolverProxy() diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index b4100271e7eb8d..3837f6390344c8 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -344,11 +344,11 @@ static CHIP_ERROR Resolve(void * context, DnssdResolveCallback callback, uint32_ ChipLogDetail(Discovery, "Resolve type=%s name=%s interface=%" PRIu32, StringOrNullMarker(type), StringOrNullMarker(name), interfaceId); - // This is a little silly, in that resolves for the sane name, type, etc get + // This is a little silly, in that resolves for the same name, type, etc get // coalesced by the underlying mDNSResponder anyway. But we need to keep // track of our context/callback/etc, (even though in practice it's always // exactly the same) and the interface id (which might actually be different - // for different Resolve calls). So for now just keep using a + // for different Resolve calls). So for now just keep using a // ResolveContext to track all that. std::shared_ptr counterHolder; if (auto existingCtx = MdnsContexts::GetInstance().GetExistingResolveForInstanceName(name)) @@ -474,15 +474,8 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * service, chip::Inet::InterfaceId inte void ChipDnssdResolveNoLongerNeeded(const char * instanceName) { auto existingCtx = MdnsContexts::GetInstance().GetExistingResolveForInstanceName(instanceName); - if (!existingCtx) - { - return; - } - - if (*existingCtx->consumerCounter == 0) - { - return; - } + VerifyOrReturn(existingCtx != nullptr); + VerifyOrReturn(*existingCtx->consumerCounter != 0); (*existingCtx->consumerCounter)--; From 0ff847175bce5963e932e60ebae780eb6cce1013 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 12 Dec 2022 13:01:45 -0500 Subject: [PATCH 3/3] Address review comments. --- src/lib/dnssd/ActiveResolveAttempts.cpp | 4 ---- src/lib/dnssd/ActiveResolveAttempts.h | 29 ++++++++++++++++++++++--- src/lib/dnssd/Resolver.h | 11 ++++++++-- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/lib/dnssd/ActiveResolveAttempts.cpp b/src/lib/dnssd/ActiveResolveAttempts.cpp index 1f41a58414d268..2065dca3440d53 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.cpp +++ b/src/lib/dnssd/ActiveResolveAttempts.cpp @@ -99,10 +99,6 @@ void ActiveResolveAttempts::NodeIdResolutionNoLongerNeeded(const PeerId & peerId if (item.attempt.Matches(peerId)) { item.attempt.ConsumerRemoved(); - if (item.attempt.ResolveData().consumerCount == 0) - { - item.attempt.Clear(); - } return; } } diff --git a/src/lib/dnssd/ActiveResolveAttempts.h b/src/lib/dnssd/ActiveResolveAttempts.h index d3b0e6b3b4900f..4a4789364d3dff 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.h +++ b/src/lib/dnssd/ActiveResolveAttempts.h @@ -72,7 +72,8 @@ class ActiveResolveAttempts ScheduledAttempt() { static_assert(sizeof(Resolve) <= sizeof(Browse) || sizeof(Resolve) <= sizeof(HeapQName), - "Figure out where to put the Resolve counter."); + "Figure out where to put the Resolve counter so that Resolve is not making ScheduledAttempt bigger than " + "it has to be anyway to handle the other attempt types."); } ScheduledAttempt(const chip::PeerId & peer, bool first) : resolveData(chip::InPlaceTemplateType(), peer), firstSend(first) @@ -186,12 +187,29 @@ class ActiveResolveAttempts bool IsIpResolve() const { return resolveData.Is(); } void Clear() { resolveData = DataType(); } + // Called when this scheduled attempt will replace an existing scheduled + // attempt, because either they match or we have run out of attempt + // slots. When this happens, we want to propagate the consumer count + // from the existing attempt to the new one, if they're matching resolve + // attempts. void WillCoalesceWith(const ScheduledAttempt & existing) { - if (IsResolve() && existing.Matches(*this)) + if (!IsResolve()) + { + // Consumer count is only tracked for resolve requests + return; + } + + if (!existing.Matches(*this)) { - resolveData.Get().consumerCount = existing.resolveData.Get().consumerCount + 1; + // Out of attempt slots, so just dropping the existing attempt. + return; } + + // Adding another consumer to the same query. Propagate along the + // consumer count to our new attempt, which will replace the + // existing one. + resolveData.Get().consumerCount = existing.resolveData.Get().consumerCount + 1; } void ConsumerRemoved() @@ -206,6 +224,11 @@ class ActiveResolveAttempts { --count; } + + if (count == 0) + { + Clear(); + } } const Browse & BrowseData() const { return resolveData.Get(); } diff --git a/src/lib/dnssd/Resolver.h b/src/lib/dnssd/Resolver.h index 094459c9b4c27d..22033b0f5c324a 100644 --- a/src/lib/dnssd/Resolver.h +++ b/src/lib/dnssd/Resolver.h @@ -388,9 +388,16 @@ class Resolver /* * Notify the resolver that one of the consumers that called ResolveNodeId * successfully no longer needs the resolution result (e.g. because it got - * the result, or got an error, or no longer cares about future updates). + * the result via OnOperationalNodeResolved, or got an via + * OnOperationalNodeResolutionFailed, or no longer cares about future + * updates). + * * There must be a NodeIdResolutionNoLongerNeeded call that matches every - * successful ResolveNodeId call. + * successful ResolveNodeId call. In particular, implementations of + * OnOperationalNodeResolved and OnOperationalNodeResolutionFailed must call + * NodeIdResolutionNoLongerNeeded once for each prior successful call to + * ResolveNodeId for the relevant PeerId that has not yet had a matching + * NodeIdResolutionNoLongerNeeded call made. */ virtual void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) = 0;