Add a AddressResolve::Resolver Shutdown command. (#16239)

* Add a AddressResolve::Resolver Shutdown command.

Ensures that we can cleanly cancel any active resolves in case
tools/commands need to shut down early.

* Also clear out the operational delegate during shutdown

* Added support for active lookup cancellation and use that in the resolver proxy

* Update the comment on Cancel to make it clear that no argument defaults was an intentional decision and somewhat discourage any future changes that may find it convenient to default to skip or cancel

* Add TODO about cancelling dnssd lookups and also added a timer adjustement on lookup cancellation

* Code review updates

src/app/OperationalDeviceProxy.cpp

@@ -40,6 +40,9 @@
 #include <system/SystemLayer.h>
 
 using namespace chip::Callback;
+using chip::AddressResolve::NodeLookupRequest;
+using chip::AddressResolve::Resolver;
+using chip::AddressResolve::ResolveResult;
 
 namespace chip {
 
@@ -309,6 +312,19 @@ CHIP_ERROR OperationalDeviceProxy::ShutdownSubscriptions()
 
 OperationalDeviceProxy::~OperationalDeviceProxy()
 {
+    if (mAddressLookupHandle.IsActive())
+    {
+        ChipLogProgress(Discovery, "Cancelling incomplete address resolution as device is being deleted.");
+
+        // Skip cancel callback since the destructor is being called, so we assume that this object is
+        // obviously not used anymore
+        CHIP_ERROR err = Resolver::Instance().CancelLookup(mAddressLookupHandle, Resolver::FailureCallback::Skip);
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(Discovery, "Lookup cancel failed: %" CHIP_ERROR_FORMAT, err.Format());
+        }
+    }
+
     if (mCASEClient)
     {
         // Make sure we don't leak it.
@@ -318,18 +334,18 @@ OperationalDeviceProxy::~OperationalDeviceProxy()
 
 CHIP_ERROR OperationalDeviceProxy::LookupPeerAddress()
 {
-    if (mAddressLookupHandle.IsInList())
+    if (mAddressLookupHandle.IsActive())
     {
         ChipLogProgress(Discovery, "Operational node lookup already in progress. Will NOT start a new one.");
         return CHIP_NO_ERROR;
     }
 
-    AddressResolve::NodeLookupRequest request(mPeerId);
+    NodeLookupRequest request(mPeerId);
 
-    return AddressResolve::Resolver::Instance().LookupNode(request, mAddressLookupHandle);
+    return Resolver::Instance().LookupNode(request, mAddressLookupHandle);
 }
 
-void OperationalDeviceProxy::OnNodeAddressResolved(const PeerId & peerId, const AddressResolve::ResolveResult & result)
+void OperationalDeviceProxy::OnNodeAddressResolved(const PeerId & peerId, const ResolveResult & result)
 {
     UpdateDeviceData(result.address, result.mrpConfig);
 }

src/controller/CHIPDeviceController.h

@@ -71,8 +71,6 @@
 #include <ble/BleLayer.h>
 #endif
 #include <controller/DeviceDiscoveryDelegate.h>
-#include <lib/dnssd/Resolver.h>
-#include <lib/dnssd/ResolverProxy.h>
 
 namespace chip {
 

src/lib/address_resolve/AddressResolve.h

@@ -81,6 +81,9 @@ class NodeLookupHandleBase : public IntrusiveListNodeBase
     void SetListener(NodeListener * listener) { mListener = listener; }
     NodeListener * GetListener() { return mListener; }
 
+    /// Convenience method that is more readable than 'IsInList'
+    inline bool IsActive() const { return IntrusiveListNodeBase::IsInList(); }
+
 protected:
     NodeListener * mListener = nullptr;
 };
@@ -168,6 +171,14 @@ class NodeLookupHandle;
 class Resolver
 {
 public:
+    /// Enumeration defining how to handle cancel callbacks during operation
+    /// cancellation.
+    enum class FailureCallback
+    {
+        Call, // Call the failure callback
+        Skip  // do not call the failure callback (generally silent operation)
+    };
+
     virtual ~Resolver();
 
     /// Expected to be called at least once before the resolver is ever
@@ -193,6 +204,21 @@ class Resolver
     ///     in progress)
     virtual CHIP_ERROR LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle) = 0;
 
+    /// Stops an active lookup request.
+    ///
+    /// Caller controlls weather the `fail` callback of the handle is invoked or not by using
+    /// the `cancel_method` argument.
+    ///
+    /// Note that there is no default cancel_method on purpose, so that the caller has to make
+    /// a clear decision if the callback should or should not be invoked.
+    virtual CHIP_ERROR CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method) = 0;
+
+    /// Shut down any active resolves
+    ///
+    /// Will immediately fail any scheduled resolve calls and will refuse to register
+    /// any new lookups until re-initialized.
+    virtual void Shutdown() = 0;
+
     /// Expected to be provided by the implementation.
     static Resolver & Instance();
 };

src/lib/address_resolve/AddressResolve_DefaultImpl.cpp

@@ -181,20 +181,70 @@ NodeLookupAction NodeLookupHandle::NextAction(System::Clock::Timestamp now)
 
 CHIP_ERROR Resolver::LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle)
 {
+    VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
     handle.ResetForLookup(mTimeSource.GetMonotonicTimestamp(), request);
     ReturnErrorOnFailure(Dnssd::Resolver::Instance().ResolveNodeId(request.GetPeerId(), Inet::IPAddressType::kAny));
     mActiveLookups.PushBack(&handle);
     ReArmTimer();
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR Resolver::CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method)
+{
+    VerifyOrReturnError(handle.IsActive(), CHIP_ERROR_INVALID_ARGUMENT);
+    mActiveLookups.Remove(&handle);
+
+    // Adjust any timing updates.
+    ReArmTimer();
+
+    if (cancel_method == FailureCallback::Call)
+    {
+        handle.GetListener()->OnNodeAddressResolutionFailed(handle.GetRequest().GetPeerId(), CHIP_ERROR_CANCELLED);
+    }
+
+    // TODO: There should be some form of cancel into Dnssd::Resolver::Instance()
+    //       to stop any resolution mechanism if applicable.
+    //
+    // Current code just removes the internal list and any callbacks of resolution will
+    // be ignored. This works from the perspective of the caller of this method,
+    // but may be wasteful by letting dnssd still work in the background.
+
+    return CHIP_NO_ERROR;
+}
+
 CHIP_ERROR Resolver::Init(System::Layer * systemLayer)
 {
     mSystemLayer = systemLayer;
     Dnssd::Resolver::Instance().SetOperationalDelegate(this);
     return CHIP_NO_ERROR;
 }
 
+void Resolver::Shutdown()
+{
+    while (mActiveLookups.begin() != mActiveLookups.end())
+    {
+        auto current = mActiveLookups.begin();
+
+        const PeerId peerId     = current->GetRequest().GetPeerId();
+        NodeListener * listener = current->GetListener();
+
+        mActiveLookups.Erase(current);
+
+        // 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)
+        listener->OnNodeAddressResolutionFailed(peerId, CHIP_ERROR_SHUT_DOWN);
+    }
+
+    // Re-arm of timer is expected to cancel any active timer as the
+    // internal list of active lookups is empty at this point.
+    ReArmTimer();
+
+    mSystemLayer = nullptr;
+    Dnssd::Resolver::Instance().SetOperationalDelegate(nullptr);
+}
+
 void Resolver::OnOperationalNodeResolved(const Dnssd::ResolvedNodeData & nodeData)
 {
     auto it = mActiveLookups.begin();

src/lib/address_resolve/AddressResolve_DefaultImpl.h

@@ -135,6 +135,8 @@ class Resolver : public ::chip::AddressResolve::Resolver, public Dnssd::Operatio
 
     CHIP_ERROR Init(System::Layer * systemLayer) override;
     CHIP_ERROR LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle) override;
+    CHIP_ERROR CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method) override;
+    void Shutdown() override;
 
     // Dnssd::OperationalResolveDelegate
 

src/lib/core/CHIPError.h

@@ -1466,8 +1466,21 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_TOO_MANY_CONNECTIONS                        CHIP_CORE_ERROR(0x72)
 
-// unused                                                      CHIP_CORE_ERROR(0x73)
-// unused                                                      CHIP_CORE_ERROR(0x74)
+/**
+ * @def CHIP_ERROR_SHUT_DOWN
+ *
+ * @brief
+ *   The operation cancelled because a shut down was initiated
+ */
+#define CHIP_ERROR_SHUT_DOWN                     CHIP_CORE_ERROR(0x73)
+
+/**
+ * @def CHIP_ERROR_SHUT_DOWN
+ *
+ * @brief
+ *   The operation has been cancelled, generally by calling a cancel/abort request.
+ */
+#define CHIP_ERROR_CANCELLED                     CHIP_CORE_ERROR(0x74)
 
 /**
  *  @def CHIP_ERROR_DRBG_ENTROPY_SOURCE_FAILED
@@ -2408,7 +2421,6 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_BUSY                     CHIP_CORE_ERROR(0xdb)
 
-
 /**
  *  @}
  */