Skip to content

Use thread local storage for cancellation none#55

Closed
bghgary wants to merge 1 commit intomicrosoft:masterfrom
bghgary:tls-cancellation-none
Closed

Use thread local storage for cancellation none#55
bghgary wants to merge 1 commit intomicrosoft:masterfrom
bghgary:tls-cancellation-none

Conversation

@bghgary
Copy link
Contributor

@bghgary bghgary commented Feb 26, 2026

Reverts #26 and replaces it with a thread local storage static variable instead. The original method leaks a 16-byte array that would be caught by memory leak detection mechanisms (e.g., _CrtDumpMemoryLeaks).

Copilot AI review requested due to automatic review settings February 26, 2026 18:36
@bghgary bghgary requested a review from ryantrem February 26, 2026 18:38
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 replaces the previous “no-destroy” implementation of arcana::cancellation::none() with a thread_local static to avoid teardown races and to prevent leak detectors from reporting the prior workaround.

Changes:

  • Remove the internal::no_destroy_cancellation wrapper and its none instance.
  • Implement cancellation::none() using a static thread_local cancellation_source.
  • Clean up a duplicate <vector> include.

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

inline cancellation& cancellation::none()
{
return internal::no_destroy_cancellation::none.get();
static thread_local cancellation_source n{};
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

This change alters the lifetime/identity of cancellation::none() (thread-local, destructed at thread exit). Please add a regression test that creates a task with cancellation::none() on one thread, runs/continues it on another thread (e.g., using a threadpool/background dispatcher), and verifies no use-after-free/assert occurs when the originating thread exits. This guards against shutdown/thread-lifetime regressions in the cancellation token implementation.

Suggested change
static thread_local cancellation_source n{};
static cancellation_source n{};

Copilot uses AI. Check for mistakes.
@@ -9,7 +9,6 @@
#include <vector>

#include <functional>
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

cancellation.h uses std::system_error/std::errc and std::mutex/std::lock_guard but does not include the defining standard headers. It currently relies on transitive includes (e.g., from ticketed_collection.h), which makes this header non-self-contained and can break builds depending on include order. Add the direct includes (e.g., <system_error> and <mutex>) here.

Suggested change
#include <functional>
#include <functional>
#include <system_error>
#include <mutex>

Copilot uses AI. Check for mistakes.
inline cancellation& cancellation::none()
{
return internal::no_destroy_cancellation::none.get();
static thread_local cancellation_source n{};
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

cancellation::none() now returns a thread_local cancellation_source. This is unsafe with the current task/cancellation API because tasks capture the cancellation token by reference (see input_output_wrapper::wrap_callable capturing &cancel), so a task created with cancellation::none() can outlive the creating thread and then dereference a destroyed thread-local object (use-after-free). It also breaks the this == &none() fast-path in add_listener() across threads (different thread-local addresses), which can cause listeners to be registered on a supposedly non-cancellable token and then trip the destructor assertion at thread exit. Prefer a single process-lifetime none object with a stable address (and ensure it is not destroyed during shutdown), or change the task API to hold the token by value/shared_ptr rather than by reference.

Suggested change
static thread_local cancellation_source n{};
// Use a single process-lifetime instance with a stable address.
// Allocated with new and intentionally never deleted to avoid
// destruction order issues at shutdown.
static cancellation& n = *new cancellation_source{};

Copilot uses AI. Check for mistakes.
@bghgary bghgary marked this pull request as draft February 26, 2026 18:48
@bghgary
Copy link
Contributor Author

bghgary commented Feb 26, 2026

This is the wrong approach.

@bghgary bghgary closed this Feb 26, 2026
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