From 882501266b1de25871b394b2a4a0be9fc9bb4c91 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 28 Apr 2023 10:56:51 +0200 Subject: [PATCH] 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 --- src/platform/tests/TestConfigurationMgr.cpp | 2 +- src/platform/tests/TestConnectivityMgr.cpp | 2 +- src/platform/tests/TestDnssd.cpp | 225 +++++++++++--------- src/platform/tests/TestKeyValueStoreMgr.cpp | 2 +- src/platform/tests/TestPlatformMgr.cpp | 2 +- src/platform/tests/TestPlatformTime.cpp | 2 +- 6 files changed, 125 insertions(+), 110 deletions(-) diff --git a/src/platform/tests/TestConfigurationMgr.cpp b/src/platform/tests/TestConfigurationMgr.cpp index aafdf4a63cfa31..dafe5b209725a2 100644 --- a/src/platform/tests/TestConfigurationMgr.cpp +++ b/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); } diff --git a/src/platform/tests/TestConnectivityMgr.cpp b/src/platform/tests/TestConnectivityMgr.cpp index 932be3dcfc5b1a..3a4548c43ef2a7 100644 --- a/src/platform/tests/TestConnectivityMgr.cpp +++ b/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); } diff --git a/src/platform/tests/TestDnssd.cpp b/src/platform/tests/TestDnssd.cpp index 4a0437ce35bfb6..d7e4ed18554f74 100644 --- a/src/platform/tests/TestDnssd.cpp +++ b/src/platform/tests/TestDnssd.cpp @@ -1,6 +1,21 @@ -#include -#include -#include +/* + * + * 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 #include @@ -14,15 +29,37 @@ 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 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(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 & addresses, CHIP_ERROR error) { + auto * ctx = static_cast(context); + auto * suite = ctx->mTestSuite; char addrBuf[100]; - nlTestSuite * suite = static_cast(context); NL_TEST_ASSERT(suite, result != nullptr); NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR); @@ -30,31 +67,42 @@ static void HandleResolve(void * context, DnssdService * result, const chip::Spa 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(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(context); + auto * ctx = static_cast(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(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(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(context); + auto * ctx = static_cast(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("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(val); - entry.mDataSize = strlen(reinterpret_cast(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 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 lock(mtx); - done = true; - doneCondition.notify_all(); - } - }); - - { - std::unique_lock 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); diff --git a/src/platform/tests/TestKeyValueStoreMgr.cpp b/src/platform/tests/TestKeyValueStoreMgr.cpp index fad7e490078069..33d70713730461 100644 --- a/src/platform/tests/TestKeyValueStoreMgr.cpp +++ b/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); } diff --git a/src/platform/tests/TestPlatformMgr.cpp b/src/platform/tests/TestPlatformMgr.cpp index 449e96d30eddbd..239817d9417465 100644 --- a/src/platform/tests/TestPlatformMgr.cpp +++ b/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); } diff --git a/src/platform/tests/TestPlatformTime.cpp b/src/platform/tests/TestPlatformTime.cpp index 95698f0d696f15..8c4e9b838e3dfe 100644 --- a/src/platform/tests/TestPlatformTime.cpp +++ b/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); }