Ensure cancellation::none implementation does not allocate (#56)

Author: bghgary

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()

Source/Shared/arcana/threading/cancellation.h

@@ -3,12 +3,11 @@
 #include "arcana/containers/ticketed_collection.h"
 
 #include <algorithm>
-#include <cassert>
 #include <atomic>
-#include <memory>
-#include <vector>
-
 #include <functional>
+#include <cassert>
+#include <memory>
+#include <optional>
 #include <vector>
 
 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,22 +59,27 @@ 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<typename CallableT>
         ticket internal_add_listener(CallableT&& callback, std::function<void()>& copied)
         {
             std::lock_guard<std::mutex> guard{ m_mutex };
 
+            if (!m_listeners.has_value())
+            {
+                m_listeners.emplace();
+            }
+
             if (m_signaled)
             {
                 copied = std::forward<CallableT>(callback);
-                return m_listeners.insert(copied, m_mutex);
+                return m_listeners->insert(copied, m_mutex);
             }
             else
             {
-                return m_listeners.insert(std::forward<CallableT>(callback), m_mutex);
+                return m_listeners->insert(std::forward<CallableT>(callback), m_mutex);
             }
         }
 
@@ -86,8 +90,11 @@ namespace arcana
             {
                 std::lock_guard<std::mutex> 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<std::function<void()>> 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<ticketed_collection<std::function<void()>>> m_listeners;
         bool m_signaled = false;
     };
 

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 <crtdbg.h>
+#include <cstdlib>
+#include <iostream>
+
+#include <arcana/threading/cancellation.h>
+
+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
+}