Add a way for Resolver consumers to cancel operational resolve attemp…

…ts. (#24010)

* 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 #23881

* Address review comments.

* Address review comments.

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

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
     {

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<NodeLookupHandle>::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)

src/lib/dnssd/ActiveResolveAttempts.cpp

@@ -92,6 +92,18 @@ 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();
+            return;
+        }
+    }
+}
+
 void ActiveResolveAttempts::MarkPending(const chip::PeerId & peerId)
 {
     MarkPending(ScheduledAttempt(peerId, /* firstSend */ true));
@@ -172,6 +184,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);

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,12 @@ 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 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<Resolve>(), peer), firstSend(first)
         {}
@@ -181,6 +187,50 @@ class ActiveResolveAttempts
         bool IsIpResolve() const { return resolveData.Is<IpResolve>(); }
         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())
+            {
+                // Consumer count is only tracked for resolve requests
+                return;
+            }
+
+            if (!existing.Matches(*this))
+            {
+                // 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<Resolve>().consumerCount = existing.resolveData.Get<Resolve>().consumerCount + 1;
+        }
+
+        void ConsumerRemoved()
+        {
+            if (!IsResolve())
+            {
+                return;
+            }
+
+            auto & count = resolveData.Get<Resolve>().consumerCount;
+            if (count > 0)
+            {
+                --count;
+            }
+
+            if (count == 0)
+            {
+                Clear();
+            }
+        }
+
         const Browse & BrowseData() const { return resolveData.Get<Browse>(); }
         const Resolve & ResolveData() const { return resolveData.Get<Resolve>(); }
         const IpResolve & IpResolveData() const { return resolveData.Get<IpResolve>(); }
@@ -208,6 +258,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

src/lib/dnssd/Discovery_ImplPlatform.cpp

@@ -625,6 +625,14 @@ CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId)
     return mResolverProxy.ResolveNodeId(peerId);
 }
 
+void DiscoveryImplPlatform::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
+{
+    char name[Common::kInstanceNameMaxLength + 1];
+    ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId));
+
+    ChipDnssdResolveNoLongerNeeded(name);
+}
+
 CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissionableNodes(DiscoveryFilter filter)
 {
     ReturnErrorOnFailure(InitImpl());
@@ -671,15 +679,29 @@ 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];
+    ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId));
+
+    ChipDnssdResolveNoLongerNeeded(name);
 }
 
 ResolverProxy::~ResolverProxy()

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;

src/lib/dnssd/Resolver.h

@@ -369,11 +369,38 @@ 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 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.  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;
+
     /**
      * Finds all commissionable nodes matching the given filter.
      *

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;

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

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;

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.
  *

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)
 {

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