Skip to content

refactor: Implement cancellation for Delay() timers.#524

Merged
beekld merged 46 commits intomainfrom
beeklimt/SDK-2212
Apr 22, 2026
Merged

refactor: Implement cancellation for Delay() timers.#524
beekld merged 46 commits intomainfrom
beeklimt/SDK-2212

Conversation

@beekld
Copy link
Copy Markdown
Contributor

@beekld beekld commented Apr 21, 2026

This implements CancellationSource / CancellationToken, based on the similar concepts in C++20 and other languages. It also updates the spots where we race timers to cancel any timers that aren't finished, so that they don't pile up internally.

This is mostly Claude-generated code, but I have read through it and verified it is correct.

There are alternatives to this model for simpler cases, but I like that this implementation means we could eventually use it for other cases (like cancelling http requests), and since all the things racing share a cancellation source, they can all be cancelled together.

This PR does not implement cancellation for HTTP requests yet, since that is much more complicated, and because they are less likely to pile up during normal operation.


Note

Medium Risk
Introduces new cross-thread cancellation primitives and changes timer scheduling/cancellation behavior, which can affect concurrency, ordering, and potential deadlocks/leaks if incorrect. Scope is limited to async utilities and FDv2 polling timeouts/delays, with added tests to validate the new behavior.

Overview
Adds a new CancellationSource/CancellationToken/CancellationCallback primitive (C++20 stop_*-style) to support coordinated cancellation with RAII callback registration and destructor synchronization.

Extends async::Delay to accept an optional CancellationToken and cancels the underlying ASIO timer when triggered, while also hardening Continuation construction to avoid self-recursion. Updates FDv2 polling (polling_synchronizer) to create a shared cancellation source for each WhenAny race and cancel the non-winning delay/timeout timers to prevent timer buildup, and adds comprehensive unit tests for cancellation and timer interaction.

Reviewed by Cursor Bugbot for commit a187254. Bugbot is set up for automated code reviews on this repo. Configure here.

beekld added 30 commits March 27, 2026 14:02
@beekld beekld requested a review from a team as a code owner April 21, 2026 00:34
Comment thread libs/internal/include/launchdarkly/async/timer.hpp Outdated
return {};
},
[executor](Continuation<void()> f) {
boost::asio::post(executor, f);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Without a move here there seems to be a recursive construction of the continuation. But maybe my test scenario isn't valid?

TEST(ContinuationRecursion, TimerCancelTriggersStackOverflow) {
    boost::asio::io_context ioc;
    auto work = boost::asio::make_work_guard(ioc);
    std::thread t([&] { ioc.run(); });

    CancellationSource cancel;

    auto future = Delay(ioc.get_executor(), std::chrono::seconds(60),
                        cancel.GetToken());
    
    std::this_thread::sleep_for(std::chrono::milliseconds(50));
    cancel.Cancel();

    const auto result = future.WaitForResult(std::chrono::seconds(5));

    // Doesn't make it here.

    ASSERT_TRUE(result.has_value());
    EXPECT_FALSE(*result);

    work.reset();
    t.join();
}
Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wow, good catch. That was kind of subtle.

Fixed.

  • Added a template constraint so that this will give a compile error instead of a runtime error.
  • Changed f to std::move(f) so it compiles.
  • Added the test above, which failed before and passes after.

Copy link
Copy Markdown
Member

@kinyoklion kinyoklion left a comment

Choose a reason for hiding this comment

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

Feedback in comment.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a187254. Configure here.

};

auto cancellation_callback =
std::make_shared<CancellationCallback>(std::move(token), cancel_timer);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing move on cancel_timer causes unnecessary copy

Low Severity

cancel_timer is passed as an lvalue to the CancellationCallback constructor, causing the lambda (and its three captures: timer, executor, timer_started_future) to be copied rather than moved into the Continuation<void()>. Since cancel_timer is never used after this line, std::move(cancel_timer) is the correct and idiomatic pattern here — consistent with std::move(token) on the same line. This is also related to the PR discussion about the recursive Continuation construction issue: while the SFINAE fix on the Continuation template constructor is the proper fix, moving here avoids the extra copy path entirely.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a187254. Configure here.

@beekld beekld merged commit 0303f66 into main Apr 22, 2026
45 checks passed
@beekld beekld deleted the beeklimt/SDK-2212 branch April 22, 2026 21:05
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