Skip to content

Ensure cancellation::none implementation does not allocate#56

Merged
bghgary merged 2 commits intomicrosoft:masterfrom
bghgary:cancellation-none-allocation-fix
Feb 26, 2026
Merged

Ensure cancellation::none implementation does not allocate#56
bghgary merged 2 commits intomicrosoft:masterfrom
bghgary:cancellation-none-allocation-fix

Conversation

@bghgary
Copy link
Contributor

@bghgary bghgary commented Feb 26, 2026

This is a fix on top of #26.

This pull request introduces a new standalone test executable for Windows platforms to detect memory leaks related to the cancellation::none() implementation. The test leverages _CrtDumpMemoryLeaks to ensure that the no-destroy pattern used in cancellation logic does not result in leaks, especially when heap allocations are involved. The test is only run in debug builds, and is integrated into the build and test system for Windows.

New memory leak detection test for cancellation logic:

  • Added a standalone executable cancellation_leak_test that verifies cancellation::none() does not cause memory leaks by invoking _CrtDumpMemoryLeaks during test execution (Source/Windows.Test/Threading/CancellationMemoryLeakTest.cpp).
  • Integrated the new test into the Windows build and test pipeline by updating CMakeLists.txt to build and run the executable as a CTest, conditional on Windows and test configuration.

Copilot AI review requested due to automatic review settings February 26, 2026 20:58
@bghgary bghgary marked this pull request as draft February 26, 2026 20:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes the cancellation class to prevent memory allocations in the cancellation::none() singleton. The none() singleton uses a no_destroy wrapper whose destructor never runs, which would cause any member allocations to leak in debug builds on some platforms. The solution wraps m_listeners in std::optional to defer allocation until a listener is actually added, which never occurs for the none() singleton.

Changes:

  • Wrapped m_listeners member in std::optional to enable lazy initialization
  • Updated all accesses to m_listeners to use optional's -> operator after checking or emplacing
  • Made throw_if_cancellation_requested() const for improved const-correctness

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bghgary bghgary force-pushed the cancellation-none-allocation-fix branch from 56fa721 to 2db4218 Compare February 26, 2026 21:02
@bghgary bghgary force-pushed the cancellation-none-allocation-fix branch from 2db4218 to 40f08b4 Compare February 26, 2026 21:28
@bghgary bghgary requested a review from Copilot February 26, 2026 21:36
@bghgary bghgary marked this pull request as ready for review February 26, 2026 21:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bghgary bghgary enabled auto-merge (squash) February 26, 2026 21:51
@bghgary bghgary requested a review from ryantrem February 26, 2026 21:54
@bghgary bghgary disabled auto-merge February 26, 2026 21:54
@bghgary bghgary merged commit 53e5b05 into microsoft:master Feb 26, 2026
13 of 19 checks passed
@bghgary bghgary deleted the cancellation-none-allocation-fix branch February 26, 2026 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants