Skip to content

Use thread local storage for cancellation none #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bghgary wants to merge 1 commit into from
Closed

Use thread local storage for cancellation none #55

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 2 additions & 28 deletions Source/Shared/arcana/threading/cancellation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
#include <vector>

namespace arcana
{
Expand Down Expand Up @@ -129,34 +128,9 @@ namespace arcana
std::atomic<bool> m_cancellationRequested{ false };
};

namespace internal::no_destroy_cancellation
{
// To address a problem with static globals recognized as the same as in a
// standards proposal, we adopt a workaround described in the proposal.
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1247r0.html
template <class T>
class no_destroy
{
alignas(T) unsigned char data_[sizeof(T)];

public:
template <class... Ts>
no_destroy(Ts&&... ts)
{
new (data_)T(std::forward<Ts>(ts)...);
}

T &get()
{
return *reinterpret_cast<T *>(data_);
}
};

inline no_destroy<cancellation_source> none{};
}

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.
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.
return n;
}
}