Fix TestDnssd exit after last HandleResolved call (#26138)

* Fix TestDnssd exit after last HandleResolved call

* Fix typo in comment

* Use dedicated context instead of global state

* Use Setup/Teardown for DNS-SD test suite

* Test timeout in case of Avahi not running

* Use ChipDnssdStopBrowse before exit

* Fix typo in comment

* Cancel timer before stopping event loop

* Avoid deadlock when stopping event loop on non-matter thread

* Update src/platform/tests/TestDnssd.cpp

* Remove unnecessary comment

Author: arkq

src/platform/tests/TestConfigurationMgr.cpp

@@ -499,7 +499,7 @@ int TestConfigurationMgr()
 {
     nlTestSuite theSuite = { "ConfigurationMgr tests", &sTests[0], TestConfigurationMgr_Setup, TestConfigurationMgr_Teardown };
 
-    // Run test suit againt one context.
+    // Run test suite against one context.
     nlTestRunner(&theSuite, nullptr);
     return nlTestRunnerStats(&theSuite);
 }

src/platform/tests/TestConnectivityMgr.cpp

@@ -99,7 +99,7 @@ int TestConnectivityMgr()
 {
     nlTestSuite theSuite = { "ConfigurationMgr tests", &sTests[0], TestConnectivityMgr_Setup, TestConnectivityMgr_Teardown };
 
-    // Run test suit againt one context.
+    // Run test suite against one context.
     nlTestRunner(&theSuite, nullptr);
     return nlTestRunnerStats(&theSuite);
 }

src/platform/tests/TestDnssd.cpp

@@ -1,6 +1,21 @@
-#include <condition_variable>
-#include <mutex>
-#include <thread>
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <atomic>
 
 #include <nlunit-test.h>
 
@@ -14,47 +29,80 @@ using chip::Dnssd::DnssdService;
 using chip::Dnssd::DnssdServiceProtocol;
 using chip::Dnssd::TextEntry;
 
-static unsigned int gBrowsedServicesCount  = 0;
-static unsigned int gResolvedServicesCount = 0;
-static bool gEndOfInput                    = false;
+namespace {
+
+struct DnssdContext
+{
+    nlTestSuite * mTestSuite;
+
+    std::atomic<bool> mTimeoutExpired{ false };
+
+    intptr_t mBrowseIdentifier = 0;
+
+    unsigned int mBrowsedServicesCount  = 0;
+    unsigned int mResolvedServicesCount = 0;
+    bool mEndOfInput                    = false;
+};
+
+} // namespace
+
+static void Timeout(chip::System::Layer * systemLayer, void * context)
+{
+    auto * ctx = static_cast<DnssdContext *>(context);
+    ChipLogError(DeviceLayer, "mDNS test timeout, is avahi daemon running?");
+    ctx->mTimeoutExpired = true;
+    chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
+}
 
 static void HandleResolve(void * context, DnssdService * result, const chip::Span<chip::Inet::IPAddress> & addresses,
                           CHIP_ERROR error)
 {
+    auto * ctx   = static_cast<DnssdContext *>(context);
+    auto * suite = ctx->mTestSuite;
     char addrBuf[100];
-    nlTestSuite * suite = static_cast<nlTestSuite *>(context);
 
     NL_TEST_ASSERT(suite, result != nullptr);
     NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR);
 
     if (!addresses.empty())
     {
         addresses.data()[0].ToString(addrBuf, sizeof(addrBuf));
-        printf("Service[%u] at [%s]:%u\n", gResolvedServicesCount, addrBuf, result->mPort);
+        printf("Service[%u] at [%s]:%u\n", ctx->mResolvedServicesCount, addrBuf, result->mPort);
     }
 
     NL_TEST_ASSERT(suite, result->mTextEntrySize == 1);
     NL_TEST_ASSERT(suite, strcmp(result->mTextEntries[0].mKey, "key") == 0);
     NL_TEST_ASSERT(suite, strcmp(reinterpret_cast<const char *>(result->mTextEntries[0].mData), "val") == 0);
 
-    if (gBrowsedServicesCount == ++gResolvedServicesCount)
+    if (ctx->mBrowsedServicesCount == ++ctx->mResolvedServicesCount)
     {
-        chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
-        chip::DeviceLayer::PlatformMgr().Shutdown();
-        exit(0);
+        chip::DeviceLayer::SystemLayer().CancelTimer(Timeout, context);
+        // StopEventLoopTask can be called from any thread, but when called from
+        // non-Matter one it will lock the Matter stack. The same locking rules
+        // are required when the resolve callback (this one) is called. In order
+        // to avoid deadlocks, we need to call the StopEventLoopTask from inside
+        // the Matter event loop by scheduling a lambda.
+        chip::DeviceLayer::SystemLayer().ScheduleLambda([]() {
+            // After last service is resolved, stop the event loop,
+            // so the test case can gracefully exit.
+            chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
+        });
     }
 }
 
 static void HandleBrowse(void * context, DnssdService * services, size_t servicesSize, bool finalBrowse, CHIP_ERROR error)
 {
-    nlTestSuite * suite = static_cast<nlTestSuite *>(context);
+    auto * ctx   = static_cast<DnssdContext *>(context);
+    auto * suite = ctx->mTestSuite;
 
     // Make sure that we will not be called again after end-of-input is set
-    NL_TEST_ASSERT(suite, gEndOfInput == false);
-    NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(suite, ctx->mEndOfInput == false);
+    // Cancelled error is expected when the browse is stopped with
+    // ChipDnssdStopBrowse(), so we will not assert on it.
+    NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR || error == CHIP_ERROR_CANCELLED);
 
-    gBrowsedServicesCount += servicesSize;
-    gEndOfInput = finalBrowse;
+    ctx->mBrowsedServicesCount += servicesSize;
+    ctx->mEndOfInput = finalBrowse;
 
     if (servicesSize > 0)
     {
@@ -63,22 +111,31 @@ static void HandleBrowse(void * context, DnssdService * services, size_t service
         {
             printf("Service[%u] name %s\n", i, services[i].mName);
             printf("Service[%u] type %s\n", i, services[i].mType);
-            NL_TEST_ASSERT(suite, ChipDnssdResolve(&services[i], services[i].mInterface, HandleResolve, suite) == CHIP_NO_ERROR);
+            NL_TEST_ASSERT(suite, ChipDnssdResolve(&services[i], services[i].mInterface, HandleResolve, context) == CHIP_NO_ERROR);
         }
     }
 }
 
-static void HandlePublish(void * context, const char * type, const char * instanceName, CHIP_ERROR error) {}
+static void DnssdErrorCallback(void * context, CHIP_ERROR error)
+{
+    auto * ctx = static_cast<DnssdContext *>(context);
+    NL_TEST_ASSERT(ctx->mTestSuite, error == CHIP_NO_ERROR);
+}
+
+static void HandlePublish(void * context, const char * type, const char * instanceName, CHIP_ERROR error)
+{
+    auto * ctx = static_cast<DnssdContext *>(context);
+    NL_TEST_ASSERT(ctx->mTestSuite, error == CHIP_NO_ERROR);
+}
 
-static void InitCallback(void * context, CHIP_ERROR error)
+static void TestDnssdPubSub_DnssdInitCallback(void * context, CHIP_ERROR error)
 {
-    DnssdService service;
-    TextEntry entry;
-    char key[]          = "key";
-    char val[]          = "val";
-    nlTestSuite * suite = static_cast<nlTestSuite *>(context);
+    auto * ctx = static_cast<DnssdContext *>(context);
+    NL_TEST_ASSERT(ctx->mTestSuite, error == CHIP_NO_ERROR);
+    auto * suite = ctx->mTestSuite;
 
-    NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR);
+    DnssdService service{};
+    TextEntry entry{ "key", reinterpret_cast<const uint8_t *>("val"), 3 };
 
     service.mInterface = chip::Inet::InterfaceId::Null();
     service.mPort      = 80;
@@ -87,107 +144,65 @@ static void InitCallback(void * context, CHIP_ERROR error)
     strcpy(service.mType, "_mock");
     service.mAddressType   = chip::Inet::IPAddressType::kAny;
     service.mProtocol      = DnssdServiceProtocol::kDnssdProtocolTcp;
-    entry.mKey             = key;
-    entry.mData            = reinterpret_cast<const uint8_t *>(val);
-    entry.mDataSize        = strlen(reinterpret_cast<const char *>(entry.mData));
     service.mTextEntries   = &entry;
     service.mTextEntrySize = 1;
     service.mSubTypes      = nullptr;
     service.mSubTypeSize   = 0;
 
-    NL_TEST_ASSERT(suite, ChipDnssdPublishService(&service, HandlePublish) == CHIP_NO_ERROR);
-    intptr_t browseIdentifier;
-    ChipDnssdBrowse("_mock", DnssdServiceProtocol::kDnssdProtocolTcp, chip::Inet::IPAddressType::kAny,
-                    chip::Inet::InterfaceId::Null(), HandleBrowse, suite, &browseIdentifier);
-}
+    NL_TEST_ASSERT(suite, ChipDnssdPublishService(&service, HandlePublish, context) == CHIP_NO_ERROR);
 
-static void ErrorCallback(void * context, CHIP_ERROR error)
-{
-    VerifyOrDieWithMsg(error == CHIP_NO_ERROR, DeviceLayer, "Mdns error: %" CHIP_ERROR_FORMAT "\n", error.Format());
+    NL_TEST_ASSERT(suite,
+                   ChipDnssdBrowse("_mock", DnssdServiceProtocol::kDnssdProtocolTcp, chip::Inet::IPAddressType::kAny,
+                                   chip::Inet::InterfaceId::Null(), HandleBrowse, context,
+                                   &ctx->mBrowseIdentifier) == CHIP_NO_ERROR);
 }
 
 void TestDnssdPubSub(nlTestSuite * inSuite, void * inContext)
 {
-    chip::Platform::MemoryInit();
-    chip::DeviceLayer::PlatformMgr().InitChipStack();
-    NL_TEST_ASSERT(inSuite, chip::Dnssd::ChipDnssdInit(InitCallback, ErrorCallback, inSuite) == CHIP_NO_ERROR);
+    DnssdContext context;
+    context.mTestSuite = inSuite;
+
+    NL_TEST_ASSERT(inSuite,
+                   chip::Dnssd::ChipDnssdInit(TestDnssdPubSub_DnssdInitCallback, DnssdErrorCallback, &context) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite,
+                   chip::DeviceLayer::SystemLayer().StartTimer(chip::System::Clock::Seconds32(5), Timeout, &context) ==
+                       CHIP_NO_ERROR);
 
     ChipLogProgress(DeviceLayer, "Start EventLoop");
     chip::DeviceLayer::PlatformMgr().RunEventLoop();
     ChipLogProgress(DeviceLayer, "End EventLoop");
-}
-
-static const nlTest sTests[] = { NL_TEST_DEF("Test Dnssd::PubSub", TestDnssdPubSub), NL_TEST_SENTINEL() };
-
-int TestDnssd()
-{
-    std::mutex mtx;
 
-    std::condition_variable readyCondition;
-    bool ready = false;
+    NL_TEST_ASSERT(inSuite, !context.mTimeoutExpired);
 
-    std::condition_variable doneCondition;
-    bool done     = false;
-    bool shutdown = false;
+    // Stop browsing so we can safely shutdown DNS-SD
+    chip::Dnssd::ChipDnssdStopBrowse(context.mBrowseIdentifier);
 
-    int retVal = EXIT_FAILURE;
-
-    std::thread t([&]() {
-        {
-            std::lock_guard<std::mutex> lock(mtx);
-            ready = true;
-            readyCondition.notify_one();
-        }
-
-        nlTestSuite theSuite = { "CHIP DeviceLayer mdns tests", &sTests[0], nullptr, nullptr };
-
-        nlTestRunner(&theSuite, nullptr);
-        retVal = nlTestRunnerStats(&theSuite);
-
-        {
-            std::lock_guard<std::mutex> lock(mtx);
-            done = true;
-            doneCondition.notify_all();
-        }
-    });
-
-    {
-        std::unique_lock<std::mutex> lock(mtx);
-        readyCondition.wait(lock, [&] { return ready; });
+    chip::Dnssd::ChipDnssdShutdown();
+}
 
-        doneCondition.wait_for(lock, std::chrono::seconds(5));
-        if (!done)
-        {
-            fprintf(stderr, "mDNS test timeout, is avahi daemon running?\n");
+static const nlTest sTests[] = { NL_TEST_DEF("Test Dnssd::PubSub", TestDnssdPubSub), NL_TEST_SENTINEL() };
 
-            //
-            // This will stop the event loop above, and wait till it has actually stopped
-            // (i.e exited RunEventLoop()).
-            //
-            chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
-            chip::Dnssd::ChipDnssdShutdown();
-            chip::DeviceLayer::PlatformMgr().Shutdown();
-            shutdown = true;
-
-            doneCondition.wait_for(lock, std::chrono::seconds(1));
-            if (!done)
-            {
-                fprintf(stderr, "Orderly shutdown of the platform main loop failed as well.\n");
-            }
-            retVal = EXIT_FAILURE;
-        }
-    }
-    t.join();
+int TestDnssd_Setup(void * inContext)
+{
+    VerifyOrReturnError(chip::Platform::MemoryInit() == CHIP_NO_ERROR, FAILURE);
+    VerifyOrReturnError(chip::DeviceLayer::PlatformMgr().InitChipStack() == CHIP_NO_ERROR, FAILURE);
+    return SUCCESS;
+}
 
-    if (!shutdown)
-    {
-        chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
-        chip::Dnssd::ChipDnssdShutdown();
-        chip::DeviceLayer::PlatformMgr().Shutdown();
-    }
+int TestDnssd_Teardown(void * inContext)
+{
+    chip::DeviceLayer::PlatformMgr().Shutdown();
     chip::Platform::MemoryShutdown();
+    return SUCCESS;
+}
+
+int TestDnssd()
+{
+    nlTestSuite theSuite = { "CHIP DeviceLayer mDNS tests", &sTests[0], TestDnssd_Setup, TestDnssd_Teardown };
 
-    return retVal;
+    // Run test suite against one context.
+    nlTestRunner(&theSuite, nullptr);
+    return nlTestRunnerStats(&theSuite);
 }
 
 CHIP_REGISTER_TEST_SUITE(TestDnssd);

src/platform/tests/TestKeyValueStoreMgr.cpp

@@ -353,7 +353,7 @@ int TestKeyValueStoreMgr()
 {
     nlTestSuite theSuite = { "KeyValueStoreMgr tests", &sTests[0], TestKeyValueStoreMgr_Setup, TestKeyValueStoreMgr_Teardown };
 
-    // Run test suit againt one context.
+    // Run test suite against one context.
     nlTestRunner(&theSuite, nullptr);
     return nlTestRunnerStats(&theSuite);
 }

src/platform/tests/TestPlatformMgr.cpp

@@ -287,7 +287,7 @@ int TestPlatformMgr()
 {
     nlTestSuite theSuite = { "PlatformMgr tests", &sTests[0], TestPlatformMgr_Setup, TestPlatformMgr_Teardown };
 
-    // Run test suit againt one context.
+    // Run test suite against one context.
     nlTestRunner(&theSuite, nullptr);
     return nlTestRunnerStats(&theSuite);
 }

src/platform/tests/TestPlatformTime.cpp

@@ -125,7 +125,7 @@ int TestPlatformTime()
 {
     nlTestSuite theSuite = { "PlatformTime tests", &sTests[0], nullptr, nullptr };
 
-    // Run test suit againt one context.
+    // Run test suite against one context.
     nlTestRunner(&theSuite, nullptr);
     return nlTestRunnerStats(&theSuite);
 }