diff --git a/CMakeLists.txt b/CMakeLists.txt index 1088839..9f59ca1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -172,4 +172,20 @@ if(ARCANA_TESTS) set_property(TARGET gmock_main PROPERTY FOLDER Dependencies) set_property(TARGET gtest PROPERTY FOLDER Dependencies) set_property(TARGET gtest_main PROPERTY FOLDER Dependencies) + + if(WIN32) + # Standalone executable that uses _CrtDumpMemoryLeaks to verify + # cancellation::none() does not leak due to the no_destroy pattern. + add_executable(cancellation_leak_test + "Source/Windows.Test/Threading/CancellationMemoryLeakTest.cpp") + + target_include_directories(cancellation_leak_test + PRIVATE "Source/Shared" + PRIVATE "Source/Windows") + + target_link_libraries(cancellation_leak_test + PRIVATE arcana) + + add_test(NAME CancellationMemoryLeakTest COMMAND cancellation_leak_test) + endif() endif() diff --git a/Source/Shared/arcana/threading/cancellation.h b/Source/Shared/arcana/threading/cancellation.h index b75ee12..fb57ee6 100644 --- a/Source/Shared/arcana/threading/cancellation.h +++ b/Source/Shared/arcana/threading/cancellation.h @@ -3,12 +3,11 @@ #include "arcana/containers/ticketed_collection.h" #include -#include #include -#include -#include - #include +#include +#include +#include #include namespace arcana @@ -25,7 +24,7 @@ namespace arcana virtual bool cancelled() const = 0; - void throw_if_cancellation_requested() + void throw_if_cancellation_requested() const { if (cancelled()) { @@ -60,7 +59,7 @@ namespace arcana virtual ~cancellation() { - assert(m_listeners.empty() && "you're destroying the listener collection and you still have listeners"); + assert((!m_listeners.has_value() || m_listeners->empty()) && "you're destroying the listener collection and you still have listeners"); } template @@ -68,14 +67,19 @@ namespace arcana { std::lock_guard guard{ m_mutex }; + if (!m_listeners.has_value()) + { + m_listeners.emplace(); + } + if (m_signaled) { copied = std::forward(callback); - return m_listeners.insert(copied, m_mutex); + return m_listeners->insert(copied, m_mutex); } else { - return m_listeners.insert(std::forward(callback), m_mutex); + return m_listeners->insert(std::forward(callback), m_mutex); } } @@ -86,8 +90,11 @@ namespace arcana { std::lock_guard guard{ m_mutex }; - listeners.reserve(m_listeners.size()); - std::copy(m_listeners.begin(), m_listeners.end(), std::back_inserter(listeners)); + if (m_listeners.has_value()) + { + listeners.reserve(m_listeners->size()); + std::copy(m_listeners->begin(), m_listeners->end(), std::back_inserter(listeners)); + } m_signaled = true; } @@ -97,13 +104,19 @@ namespace arcana // then a child function does the same, the child // cancellation runs first. This avoids ownership // semantic issues. - for(auto itr = listeners.rbegin(); itr != listeners.rend(); ++itr) + for (auto itr = listeners.rbegin(); itr != listeners.rend(); ++itr) + { (*itr)(); + } } private: mutable std::mutex m_mutex; - ticketed_collection> m_listeners; + // std::optional is used here because the none() singleton is held in a no_destroy wrapper whose destructor + // never runs. The underlying std::vector inside ticketed_collection allocates memory in debug builds on some + // platforms, and that allocation would be a leak since no_destroy never frees it. Using std::optional will + // delay the allocation until it's actually needed, which is never for the none() singleton. + std::optional>> m_listeners; bool m_signaled = false; }; diff --git a/Source/Windows.Test/Threading/CancellationMemoryLeakTest.cpp b/Source/Windows.Test/Threading/CancellationMemoryLeakTest.cpp new file mode 100644 index 0000000..b6231df --- /dev/null +++ b/Source/Windows.Test/Threading/CancellationMemoryLeakTest.cpp @@ -0,0 +1,37 @@ +// +// Copyright (C) Microsoft Corporation. All rights reserved. +// + +// Standalone executable that verifies cancellation::none() does not cause memory +// leaks. cancellation::none() is backed by a no_destroy wrapper whose destructor +// never runs. If the cancellation_source constructor allocates heap memory (e.g. +// an eagerly-constructed std::vector inside ticketed_collection), that memory can +// never be freed and will be reported by _CrtDumpMemoryLeaks. +// +// Including cancellation.h brings the no_destroy inline variable definition into +// this translation unit, so it is constructed during static initialization before +// main runs. We call _CrtDumpMemoryLeaks explicitly and return a non-zero exit +// code so that CTest treats the test as failed when leaks are detected. + +#include +#include +#include + +#include + +int main() +{ +#ifdef _DEBUG + // _CrtDumpMemoryLeaks reports all CRT debug-heap blocks still allocated. + if (_CrtDumpMemoryLeaks()) + { + std::cerr << "Memory leaks detected!" << std::endl; + return EXIT_FAILURE; + } + + return EXIT_SUCCESS; +#else + std::cerr << "Skipping: _CrtDumpMemoryLeaks is a no-op in Release builds." << std::endl; + return EXIT_SUCCESS; +#endif +}