[DNS-SD] Redesign ServiceAdvertiser and Resolver interfaces (#10181)

* [DNS-SD] Redesign ServiceAdvertiser and Resolver interfaces

* Change Resolver::SetResolverDelegate's return type to void.
* Replace ServiceAdvertiser::Start with Init, and
  ServiceAdvertiser::StopPublishDevice with RemoveServices as
  the purpose of the former methods was unclear and the code
  in DiscoveryImplPlatform::Start would do the same as in
  DiscoveryImplPlatform::StopPublishDevice. Now Init is
  meant for one-time operations and RemoveServices for
  removing advertised services so they can be refreshed.
* Likewise, replace Resolver::Start with Init.
* Add ServiceAdvertiser::CompleteServiceUpdate which should
  be called after publishing all desired services with
  Advertise methods. This may come in handy in other
  platforms, too, but is particularly useful for Thread-based
  platforms using SRP instead of mDNS. Such platforms  cannot
  immediately remove and re-add DNS services. Instead, they
  must communicate with the SRP server to complete any of
  these operations. CompleteServiceUpdate method allows
  Thread-based platform to remove services which have been
  marked for removal with the RemoveServices method and have
  not been re-added with subsequent Advertise calls.

* Address some review comments

* Rename CompleteServiceUpdate to FinalizeServiceUpdate

examples/chip-tool/commands/discover/Commands.h

@@ -24,8 +24,6 @@
 #include <controller/DeviceAddressUpdateDelegate.h>
 #include <lib/mdns/Resolver.h>
 
-constexpr uint16_t kMdnsPort = 5353;
-
 class Resolve : public DiscoverCommand, public chip::Mdns::ResolverDelegate
 {
 public:
@@ -34,9 +32,8 @@ class Resolve : public DiscoverCommand, public chip::Mdns::ResolverDelegate
     /////////// DiscoverCommand Interface /////////
     CHIP_ERROR RunCommand(NodeId remoteId, uint64_t fabricId) override
     {
-        ReturnErrorOnFailure(chip::Mdns::Resolver::Instance().StartResolver(&chip::DeviceLayer::InetLayer, kMdnsPort));
-        ReturnErrorOnFailure(chip::Mdns::Resolver::Instance().SetResolverDelegate(nullptr));
-        ReturnErrorOnFailure(chip::Mdns::Resolver::Instance().SetResolverDelegate(this));
+        ReturnErrorOnFailure(chip::Mdns::Resolver::Instance().Init(&chip::DeviceLayer::InetLayer));
+        chip::Mdns::Resolver::Instance().SetResolverDelegate(this);
         ChipLogProgress(chipTool, "Mdns: Searching for NodeId: %" PRIx64 " FabricId: %" PRIx64 " ...", remoteId, fabricId);
         return chip::Mdns::Resolver::Instance().ResolveNodeId(chip::PeerId().SetNodeId(remoteId).SetCompressedFabricId(fabricId),
                                                               chip::Inet::kIPAddressType_Any);

examples/minimal-mdns/advertiser.cpp

@@ -255,7 +255,7 @@ int main(int argc, char ** args)
         return 1;
     }
 
-    if (chip::Mdns::ServiceAdvertiser::Instance().Start(&DeviceLayer::InetLayer, Mdns::kMdnsPort) != CHIP_NO_ERROR)
+    if (chip::Mdns::ServiceAdvertiser::Instance().Init(&DeviceLayer::InetLayer) != CHIP_NO_ERROR)
     {
         fprintf(stderr, "FAILED to start MDNS advertisement\n");
         return 1;

src/app/server/Mdns.cpp

@@ -148,17 +148,17 @@ bool MdnsServer::OnExpiration(uint64_t expirationMs)
 
     ChipLogDetail(Discovery, "OnExpiration - valid time out");
 
-    // reset advertising
-    CHIP_ERROR err;
-    err = chip::Mdns::ServiceAdvertiser::Instance().StopPublishDevice();
+    CHIP_ERROR err = Mdns::ServiceAdvertiser::Instance().Init(&chip::DeviceLayer::InetLayer);
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(Discovery, "Failed to stop ServiceAdvertiser: %s", chip::ErrorStr(err));
+        ChipLogError(Discovery, "Failed to initialize advertiser: %s", chip::ErrorStr(err));
     }
-    err = chip::Mdns::ServiceAdvertiser::Instance().Start(&chip::DeviceLayer::InetLayer, chip::Mdns::kMdnsPort);
+
+    // reset advertising
+    err = Mdns::ServiceAdvertiser::Instance().RemoveServices();
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(Discovery, "Failed to start ServiceAdvertiser: %s", chip::ErrorStr(err));
+        ChipLogError(Discovery, "Failed to remove advertised services: %s", chip::ErrorStr(err));
     }
 
     // restart operational (if needed)
@@ -176,6 +176,12 @@ bool MdnsServer::OnExpiration(uint64_t expirationMs)
     }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
 
+    err = Mdns::ServiceAdvertiser::Instance().FinalizeServiceUpdate();
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(Discovery, "Failed to finalize service update: %s", chip::ErrorStr(err));
+    }
+
     return true;
 }
 
@@ -406,17 +412,16 @@ void MdnsServer::StartServer(chip::Mdns::CommissioningMode mode)
 
     ClearTimeouts();
 
-    CHIP_ERROR err;
-    err = chip::Mdns::ServiceAdvertiser::Instance().StopPublishDevice();
+    CHIP_ERROR err = Mdns::ServiceAdvertiser::Instance().Init(&chip::DeviceLayer::InetLayer);
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(Discovery, "Failed to stop ServiceAdvertiser: %s", chip::ErrorStr(err));
+        ChipLogError(Discovery, "Failed to initialize advertiser: %s", chip::ErrorStr(err));
     }
 
-    err = chip::Mdns::ServiceAdvertiser::Instance().Start(&chip::DeviceLayer::InetLayer, chip::Mdns::kMdnsPort);
+    err = Mdns::ServiceAdvertiser::Instance().RemoveServices();
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(Discovery, "Failed to start ServiceAdvertiser: %s", chip::ErrorStr(err));
+        ChipLogError(Discovery, "Failed to remove advertised services: %s", chip::ErrorStr(err));
     }
 
     err = AdvertiseOperational();
@@ -471,6 +476,12 @@ void MdnsServer::StartServer(chip::Mdns::CommissioningMode mode)
         ChipLogError(Discovery, "Failed to advertise commissioner: %s", chip::ErrorStr(err));
     }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
+
+    err = Mdns::ServiceAdvertiser::Instance().FinalizeServiceUpdate();
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(Discovery, "Failed to finalize service update: %s", chip::ErrorStr(err));
+    }
 }
 
 #if CHIP_ENABLE_ROTATING_DEVICE_ID

src/app/server/Server.cpp

@@ -187,7 +187,7 @@ CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint
 
 void Server::Shutdown()
 {
-    chip::Mdns::ServiceAdvertiser::Instance().StopPublishDevice();
+    chip::Mdns::ServiceAdvertiser::Instance().Shutdown();
     chip::app::InteractionModelEngine::GetInstance()->Shutdown();
     mExchangeMgr.Shutdown();
     mSessions.Shutdown();

src/controller/AbstractMdnsDiscoveryController.cpp

@@ -66,10 +66,10 @@ void AbstractMdnsDiscoveryController::OnNodeDiscoveryComplete(const chip::Mdns::
 
 CHIP_ERROR AbstractMdnsDiscoveryController::SetUpNodeDiscovery()
 {
-    ReturnErrorOnFailure(mResolver->SetResolverDelegate(this));
 #if CONFIG_DEVICE_LAYER
-    ReturnErrorOnFailure(mResolver->StartResolver(&DeviceLayer::InetLayer, kMdnsPort));
+    ReturnErrorOnFailure(mResolver->Init(&DeviceLayer::InetLayer));
 #endif
+    mResolver->SetResolverDelegate(this);
 
     auto discoveredNodes = GetDiscoveredNodes();
     for (auto & discoveredNode : discoveredNodes)

src/controller/AbstractMdnsDiscoveryController.h

@@ -27,8 +27,6 @@ namespace chip {
 
 namespace Controller {
 
-constexpr uint16_t kMdnsPort = 5353;
-
 /**
  * @brief
  *   Convenient superclass for controller implementations that need to discover

src/controller/CHIPDeviceController.cpp

@@ -135,10 +135,10 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
     params.systemState->ExchangeMgr()->SetDelegate(this);
 
 #if CHIP_DEVICE_CONFIG_ENABLE_MDNS
-    ReturnErrorOnFailure(Mdns::Resolver::Instance().SetResolverDelegate(this));
+    Mdns::Resolver::Instance().Init(params.systemState->InetLayer());
+    Mdns::Resolver::Instance().SetResolverDelegate(this);
     RegisterDeviceAddressUpdateDelegate(params.deviceAddressUpdateDelegate);
     RegisterDeviceDiscoveryDelegate(params.deviceDiscoveryDelegate);
-    Mdns::Resolver::Instance().StartResolver(params.systemState->InetLayer(), kMdnsPort);
 #endif // CHIP_DEVICE_CONFIG_ENABLE_MDNS
 
     InitDataModelHandler(params.systemState->ExchangeMgr());
@@ -217,7 +217,7 @@ CHIP_ERROR DeviceController::Shutdown()
     mState = State::NotInitialized;
 
 #if CHIP_DEVICE_CONFIG_ENABLE_MDNS
-    Mdns::Resolver::Instance().ShutdownResolver();
+    Mdns::Resolver::Instance().Shutdown();
 #endif // CHIP_DEVICE_CONFIG_ENABLE_MDNS
 
     mStorageDelegate = nullptr;

src/controller/python/chip/discovery/NodeResolution.cpp

@@ -33,8 +33,6 @@ namespace {
 using DiscoverSuccessCallback = void (*)(uint64_t fabricId, uint64_t nodeId, uint32_t interfaceId, const char * ip, uint16_t port);
 using DiscoverFailureCallback = void (*)(uint64_t fabricId, uint64_t nodeId, ChipError::StorageType error_code);
 
-constexpr uint16_t kMdnsPort = 5353;
-
 class PythonResolverDelegate : public ResolverDelegate
 {
 public:
@@ -95,11 +93,9 @@ extern "C" ChipError::StorageType pychip_discovery_resolve(uint64_t fabricId, ui
     CHIP_ERROR result = CHIP_NO_ERROR;
 
     chip::python::ChipMainThreadScheduleAndWait([&] {
-        result = Resolver::Instance().StartResolver(&chip::DeviceLayer::InetLayer, kMdnsPort);
-        ReturnOnFailure(result);
-
-        result = Resolver::Instance().SetResolverDelegate(&gPythonResolverDelegate);
+        result = Resolver::Instance().Init(&chip::DeviceLayer::InetLayer);
         ReturnOnFailure(result);
+        Resolver::Instance().SetResolverDelegate(&gPythonResolverDelegate);
 
         result = Resolver::Instance().ResolveNodeId(chip::PeerId().SetCompressedFabricId(fabricId).SetNodeId(nodeId),
                                                     chip::Inet::IPAddressType::kIPAddressType_Any);

src/controller/tests/TestCommissionableNodeController.cpp

@@ -31,17 +31,16 @@ namespace {
 class MockResolver : public Resolver
 {
 public:
-    CHIP_ERROR SetResolverDelegate(ResolverDelegate *) override { return SetResolverDelegateStatus; }
-    CHIP_ERROR StartResolver(chip::Inet::InetLayer * inetLayer, uint16_t port) override { return StartResolverStatus; }
-    void ShutdownResolver() override {}
+    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 FindCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return FindCommissionersStatus; }
     CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; }
 
-    CHIP_ERROR SetResolverDelegateStatus = CHIP_NO_ERROR;
-    CHIP_ERROR StartResolverStatus       = CHIP_NO_ERROR;
-    CHIP_ERROR ResolveNodeIdStatus       = CHIP_NO_ERROR;
-    CHIP_ERROR FindCommissionersStatus   = CHIP_NO_ERROR;
+    CHIP_ERROR InitStatus              = CHIP_NO_ERROR;
+    CHIP_ERROR ResolveNodeIdStatus     = CHIP_NO_ERROR;
+    CHIP_ERROR FindCommissionersStatus = CHIP_NO_ERROR;
 };
 
 } // namespace
@@ -143,18 +142,10 @@ void TestDiscoverCommissioners_HappyCaseWithDiscoveryFilter(nlTestSuite * inSuit
                        CHIP_NO_ERROR);
 }
 
-void TestDiscoverCommissioners_SetResolverDelegateError_ReturnsError(nlTestSuite * inSuite, void * inContext)
+void TestDiscoverCommissioners_InitError_ReturnsError(nlTestSuite * inSuite, void * inContext)
 {
     MockResolver resolver;
-    resolver.SetResolverDelegateStatus = CHIP_ERROR_INTERNAL;
-    CommissionableNodeController controller(&resolver);
-    NL_TEST_ASSERT(inSuite, controller.DiscoverCommissioners() != CHIP_NO_ERROR);
-}
-
-void TestDiscoverCommissioners_StartResolverError_ReturnsError(nlTestSuite * inSuite, void * inContext)
-{
-    MockResolver resolver;
-    resolver.StartResolverStatus = CHIP_ERROR_INTERNAL;
+    resolver.InitStatus = CHIP_ERROR_INTERNAL;
     CommissionableNodeController controller(&resolver);
     NL_TEST_ASSERT(inSuite, controller.DiscoverCommissioners() != CHIP_NO_ERROR);
 }
@@ -178,8 +169,7 @@ const nlTest sTests[] =
     NL_TEST_DEF("TestGetDiscoveredCommissioner_NoNodesDiscovered_ReturnsNullptr", TestGetDiscoveredCommissioner_NoNodesDiscovered_ReturnsNullptr),
     NL_TEST_DEF("TestDiscoverCommissioners_HappyCase", TestDiscoverCommissioners_HappyCase),
     NL_TEST_DEF("TestDiscoverCommissioners_HappyCaseWithDiscoveryFilter", TestDiscoverCommissioners_HappyCaseWithDiscoveryFilter),
-    NL_TEST_DEF("TestDiscoverCommissioners_SetResolverDelegateError_ReturnsError", TestDiscoverCommissioners_SetResolverDelegateError_ReturnsError),
-    NL_TEST_DEF("TestDiscoverCommissioners_StartResolverError_ReturnsError", TestDiscoverCommissioners_StartResolverError_ReturnsError),
+    NL_TEST_DEF("TestDiscoverCommissioners_InitError_ReturnsError", TestDiscoverCommissioners_InitError_ReturnsError),
     NL_TEST_DEF("TestDiscoverCommissioners_FindCommissionersError_ReturnsError", TestDiscoverCommissioners_FindCommissionersError_ReturnsError),
     NL_TEST_SENTINEL()
 };

src/include/platform/ThreadStackManager.h

@@ -99,7 +99,8 @@ class ThreadStackManager
                              const Span<const char * const> & aSubTypes, const Span<const Mdns::TextEntry> & aTxtEntries,
                              uint32_t aLeaseInterval, uint32_t aKeyLeaseInterval);
     CHIP_ERROR RemoveSrpService(const char * aInstanceName, const char * aName);
-    CHIP_ERROR RemoveAllSrpServices();
+    CHIP_ERROR InvalidateAllSrpServices(); ///< Mark all SRP services as invalid
+    CHIP_ERROR RemoveInvalidSrpServices(); ///< Remove SRP services marked as invalid
     CHIP_ERROR SetupSrpHost(const char * aHostName);
 
 #if CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT
@@ -259,9 +260,14 @@ inline CHIP_ERROR ThreadStackManager::RemoveSrpService(const char * aInstanceNam
     return static_cast<ImplClass *>(this)->_RemoveSrpService(aInstanceName, aName);
 }
 
-inline CHIP_ERROR ThreadStackManager::RemoveAllSrpServices()
+inline CHIP_ERROR ThreadStackManager::InvalidateAllSrpServices()
 {
-    return static_cast<ImplClass *>(this)->_RemoveAllSrpServices();
+    return static_cast<ImplClass *>(this)->_InvalidateAllSrpServices();
+}
+
+inline CHIP_ERROR ThreadStackManager::RemoveInvalidSrpServices()
+{
+    return static_cast<ImplClass *>(this)->_RemoveInvalidSrpServices();
 }
 
 inline CHIP_ERROR ThreadStackManager::SetupSrpHost(const char * aHostName)

src/lib/mdns/Advertiser.h

@@ -285,30 +285,68 @@ class CommissionAdvertisingParameters : public BaseAdvertisingParams<CommissionA
     bool mPairingInstrHasValue = false;
 };
 
-/// Handles advertising of CHIP nodes
+/**
+ * Interface for advertising CHIP DNS-SD services.
+ *
+ * A user of this interface must first initialize the advertiser using the `Init` method.
+ *
+ * Then, whenever advertised services need to be refreshed, the following sequence of events must
+ * occur:
+ * 1. Call the `RemoveServices` method.
+ * 2. Call one of the `Advertise` methods for each service to be added or updated.
+ * 3. Call the `FinalizeServiceUpdate` method to finalize the update and apply all pending changes.
+ */
 class ServiceAdvertiser
 {
 public:
     virtual ~ServiceAdvertiser() {}
 
-    /// Starts the advertiser. Items 'Advertised' will become visible.
-    /// Must be called before Advertise() calls.
-    virtual CHIP_ERROR Start(chip::Inet::InetLayer * inetLayer, uint16_t port) = 0;
-
-    /// Advertises the CHIP node as an operational node
+    /**
+     * Initializes the advertiser.
+     *
+     * The method must be called before other methods of this class.
+     * If the advertiser has already been initialized, the method exits immediately with no error.
+     */
+    virtual CHIP_ERROR Init(chip::Inet::InetLayer * inetLayer) = 0;
+
+    /**
+     * Shuts down the advertiser.
+     */
+    virtual void Shutdown() = 0;
+
+    /**
+     * Removes or marks all services being advertised for removal.
+     *
+     * Depending on the implementation, the method may either stop advertising existing services
+     * immediately, or mark them for removal upon the subsequent `FinalizeServiceUpdate` method call.
+     */
+    virtual CHIP_ERROR RemoveServices() = 0;
+
+    /**
+     * Advertises the given operational node service.
+     */
     virtual CHIP_ERROR Advertise(const OperationalAdvertisingParameters & params) = 0;
 
-    /// Advertises the CHIP node as a commissioner/commissionable node
+    /**
+     * Advertises the given commissionable/commissioner node service.
+     */
     virtual CHIP_ERROR Advertise(const CommissionAdvertisingParameters & params) = 0;
 
-    /// Stops the advertiser.
-    virtual CHIP_ERROR StopPublishDevice() = 0;
+    /**
+     * Finalizes updating advertised services.
+     *
+     * This method can be used by some implementations to apply changes made with the `RemoveServices`
+     * and `Advertise` methods in case they could not be applied immediately.
+     */
+    virtual CHIP_ERROR FinalizeServiceUpdate() = 0;
+
+    /**
+     * Returns the commissionable node service instance name formatted as hex string.
+     */
+    virtual CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) = 0;
 
     /// Provides the system-wide implementation of the service advertiser
     static ServiceAdvertiser & Instance();
-
-    /// Returns DNS-SD instance name formatted as hex string
-    virtual CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) = 0;
 };
 
 } // namespace Mdns

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp

@@ -115,10 +115,12 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
     ~AdvertiserMinMdns() {}
 
     // Service advertiser
-    CHIP_ERROR Start(chip::Inet::InetLayer * inetLayer, uint16_t port) override;
+    CHIP_ERROR Init(chip::Inet::InetLayer * inetLayer) override;
+    void Shutdown() override;
+    CHIP_ERROR RemoveServices() override;
     CHIP_ERROR Advertise(const OperationalAdvertisingParameters & params) override;
     CHIP_ERROR Advertise(const CommissionAdvertisingParameters & params) override;
-    CHIP_ERROR StopPublishDevice() override;
+    CHIP_ERROR FinalizeServiceUpdate() override { return CHIP_NO_ERROR; }
     CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) override;
 
     // MdnsPacketDelegate
@@ -272,7 +274,7 @@ void AdvertiserMinMdns::OnQuery(const QueryData & data)
     }
 }
 
-CHIP_ERROR AdvertiserMinMdns::Start(chip::Inet::InetLayer * inetLayer, uint16_t port)
+CHIP_ERROR AdvertiserMinMdns::Init(chip::Inet::InetLayer * inetLayer)
 {
     GlobalMinimalMdnsServer::Server().Shutdown();
 
@@ -282,7 +284,7 @@ CHIP_ERROR AdvertiserMinMdns::Start(chip::Inet::InetLayer * inetLayer, uint16_t
     // GlobalMinimalMdnsServer (used for testing).
     mResponseSender.SetServer(&GlobalMinimalMdnsServer::Server());
 
-    ReturnErrorOnFailure(GlobalMinimalMdnsServer::Instance().StartServer(inetLayer, port));
+    ReturnErrorOnFailure(GlobalMinimalMdnsServer::Instance().StartServer(inetLayer, kMdnsPort));
 
     ChipLogProgress(Discovery, "CHIP minimal mDNS started advertising.");
 
@@ -291,8 +293,12 @@ CHIP_ERROR AdvertiserMinMdns::Start(chip::Inet::InetLayer * inetLayer, uint16_t
     return CHIP_NO_ERROR;
 }
 
-/// Stops the advertiser.
-CHIP_ERROR AdvertiserMinMdns::StopPublishDevice()
+void AdvertiserMinMdns::Shutdown()
+{
+    GlobalMinimalMdnsServer::Server().Shutdown();
+}
+
+CHIP_ERROR AdvertiserMinMdns::RemoveServices()
 {
     for (auto & allocator : mQueryResponderAllocatorOperational)
     {