-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: Implement cancellation for Delay() timers. #524
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
bf945b3
refactor: add Apply method to MemoryStore for transactional change sets
beekld cc59635
fix an unneeded copy, and warn on missing enum case
beekld 82d09d2
add an ApplyResult with the changed keys
beekld 259d8e9
mark the apply result as nodiscard for now
beekld f5b82bf
simplify tests
beekld 7826357
simplify, since FDv2 doesnt require version checking in memory store
beekld 651a780
refactor: define new Synchronizer and Initializer interfaces for FDv2
beekld fe37f55
move selector source into Next
beekld c470763
Merge branch 'main' into beeklimt/SDK-2096
beekld ed22857
distinguish between timeouts and errors
beekld f0013da
refactor: implement fdv2 polling initializer / synchronizer
beekld 29a94c8
update for upstream change
beekld 9e75b47
Merge branch 'main' into beeklimt/SDK-2096
beekld c8e1c7f
Merge branch 'beeklimt/SDK-2096' into beeklimt/SDK-2097
beekld 0996cca
refactor: fix an error type that could be better
beekld 92489a6
refactor polling synchronizer to make it more threadsafe
beekld 071a4eb
update asio_requester to use new style
beekld f1b3f06
add comments
beekld 37fb4c0
fix missing kInactive handling
beekld 07810cd
refactor error types to preserve more info
beekld 55e4d8c
refactor shared code into a separate file
beekld b8a334c
refactor FDv2ProtocolHandler::HandleEvent to be clearer
beekld 6e5b1d4
clang format
beekld 1e2d438
handle closed first in network response
beekld 01cc26b
handle url parsing failure
beekld 7a60c2a
minor cleanup and add tests
beekld c8e736f
clang-format
beekld 19da904
Merge branch 'main' into beeklimt/SDK-2097
beekld 48989a4
rewrite everything to use promises.
beekld 45befda
fix: make it safe to delete the synchronizer at any time
beekld cd0914d
refactor: reorder and clean up code
beekld 9d08671
docs: fix some comments
beekld 20e3f96
refactor: some simple optimizations
beekld d37dc5d
refactor: more cleanup
beekld 3378ed1
fix: close the synchronizer on destruction
beekld 66e3936
fix: minor feedback fix
beekld e9afed0
docs: fix some comments
beekld cca8ea9
feat: add the concept of cancellation tokens
beekld 73a2716
fix: cancel timers when one future wins a race
beekld 112c78e
refactor: use generic http requester interface
beekld 4b02b67
refactor: break up HandleFDv2PollResponse into separate functions
beekld 4bfd3aa
Merge branch 'main' into beeklimt/SDK-2097
beekld 2b4aa27
Merge branch 'beeklimt/SDK-2097' into beeklimt/SDK-2212
beekld a6a2757
remove some superfluous comments
beekld ca58a13
fix: fix a race condition with cancellation
beekld a187254
fix: fix an issue with an infinite recursion in timer cancellation
beekld File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
242 changes: 242 additions & 0 deletions
242
libs/internal/include/launchdarkly/async/cancellation.hpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,242 @@ | ||
| #pragma once | ||
|
|
||
| #include <launchdarkly/async/promise.hpp> | ||
|
|
||
| #include <condition_variable> | ||
| #include <map> | ||
| #include <memory> | ||
| #include <mutex> | ||
| #include <thread> | ||
|
|
||
| // This file implements a cancellation primitive modelled after C++20's | ||
| // std::stop_source / std::stop_token / std::stop_callback design: a source | ||
| // owns the ability to trigger cancellation, lightweight tokens (derived from | ||
| // the source) can be freely passed around, and CancellationCallback provides | ||
| // RAII registration of a callback tied to a token. | ||
|
|
||
| namespace launchdarkly::async { | ||
|
|
||
| // CancellationState is the shared state between a CancellationSource and all | ||
| // CancellationTokens and CancellationCallbacks derived from it. This is an | ||
| // internal class; use CancellationSource, CancellationToken, and | ||
| // CancellationCallback instead. | ||
| class CancellationState { | ||
| public: | ||
| using CallbackId = std::size_t; | ||
|
|
||
| // Sentinel returned by Register() when the state was already cancelled; | ||
| // the callback is invoked immediately in that case. Deregister() is a | ||
| // no-op for this value. | ||
| static constexpr CallbackId kAlreadyCancelled = 0; | ||
|
|
||
| CancellationState() = default; | ||
| ~CancellationState() = default; | ||
| CancellationState(CancellationState const&) = delete; | ||
| CancellationState& operator=(CancellationState const&) = delete; | ||
| CancellationState(CancellationState&&) = delete; | ||
| CancellationState& operator=(CancellationState&&) = delete; | ||
|
|
||
| // Registers a callback and returns its ID. If Cancel() has already been | ||
| // called, invokes cb immediately (outside the lock) and returns | ||
| // kAlreadyCancelled. | ||
| CallbackId Register(Continuation<void()> cb) { | ||
| std::unique_lock lock(mutex_); | ||
| if (cancelled_) { | ||
| lock.unlock(); | ||
| cb(); | ||
| return kAlreadyCancelled; | ||
| } | ||
| CallbackId id = next_id_++; | ||
| callbacks_.emplace(id, std::move(cb)); | ||
| return id; | ||
| } | ||
|
|
||
| // Deregisters the callback with the given ID. If the callback is currently | ||
| // executing on another thread, blocks until execution completes. This | ||
| // mirrors the synchronization guarantee of C++20's stop_callback | ||
| // destructor: after Deregister returns, the callback is guaranteed to have | ||
| // either never run or fully completed. No-op if id is kAlreadyCancelled or | ||
| // the callback has already run. | ||
| void Deregister(CallbackId id) { | ||
| if (id == kAlreadyCancelled) { | ||
| return; | ||
| } | ||
|
|
||
| std::unique_lock lock(mutex_); | ||
|
|
||
| // The callback is still pending: remove it before it can run. | ||
| if (callbacks_.erase(id)) { | ||
| return; | ||
| } | ||
|
|
||
| // The callback has already run, or was never registered. | ||
| if (executing_id_ != id) { | ||
| return; | ||
| } | ||
|
|
||
| // The callback is executing on this thread (re-entrant call from | ||
| // within the callback itself): return without waiting to avoid | ||
| // deadlock. | ||
| if (executing_thread_ == std::this_thread::get_id()) { | ||
| return; | ||
| } | ||
|
|
||
| // The callback is executing on another thread. Wait for it to | ||
| // finish. executing_id_ is set while the state lock is held — before | ||
| // unlocking for invocation — so there is no window where the callback | ||
| // is running but executing_id_ is not yet set. | ||
| executing_done_.wait(lock, [this, id] { return executing_id_ != id; }); | ||
| } | ||
|
|
||
| // Invokes all registered callbacks in registration order, then clears the | ||
| // pending list. Callbacks are executed one at a time with the lock | ||
| // released during each invocation to prevent deadlocks. No-op if called | ||
| // more than once. | ||
| void Cancel() { | ||
| std::unique_lock lock(mutex_); | ||
| if (cancelled_) { | ||
| return; | ||
| } | ||
| cancelled_ = true; | ||
|
|
||
| while (!callbacks_.empty()) { | ||
| // Extract the next entry while still holding the lock, then set | ||
| // executing_id_ before releasing. This ensures Deregister can | ||
| // never observe a window where the callback is running but | ||
| // executing_id_ is unset. | ||
| auto node = callbacks_.extract(callbacks_.begin()); | ||
| executing_id_ = node.key(); | ||
| executing_thread_ = std::this_thread::get_id(); | ||
|
|
||
| lock.unlock(); | ||
| node.mapped()(); | ||
| lock.lock(); | ||
|
|
||
| executing_id_ = kAlreadyCancelled; | ||
| lock.unlock(); | ||
| executing_done_.notify_all(); | ||
| lock.lock(); | ||
| } | ||
| } | ||
|
|
||
| // Returns true if Cancel() has been called. | ||
| bool IsCancelled() const { | ||
| std::lock_guard lock(mutex_); | ||
| return cancelled_; | ||
| } | ||
|
|
||
| private: | ||
| mutable std::mutex mutex_; | ||
| bool cancelled_ = false; | ||
| CallbackId next_id_ = 1; // Real IDs start at 1; 0 is kAlreadyCancelled. | ||
| std::map<CallbackId, Continuation<void()>> callbacks_; | ||
|
|
||
| // Tracks which callback (if any) is currently being invoked by Cancel(), | ||
| // and on which thread, to support the blocking destructor in Deregister. | ||
| CallbackId executing_id_ = kAlreadyCancelled; | ||
| std::thread::id executing_thread_; | ||
| std::condition_variable executing_done_; | ||
| }; | ||
|
|
||
| class CancellationToken; | ||
|
|
||
| // CancellationSource is the write end of a cancellation pair: call Cancel() | ||
| // to signal all operations holding tokens derived from this source. | ||
| // | ||
| // CancellationSource is copyable; copies share the same underlying | ||
| // CancellationState, matching the behaviour of C++20's stop_source. | ||
| class CancellationSource { | ||
| public: | ||
| CancellationSource() : state_(std::make_shared<CancellationState>()) {} | ||
|
|
||
| ~CancellationSource() = default; | ||
| CancellationSource(CancellationSource const&) = default; | ||
| CancellationSource& operator=(CancellationSource const&) = default; | ||
| CancellationSource(CancellationSource&&) = default; | ||
| CancellationSource& operator=(CancellationSource&&) = default; | ||
|
|
||
| // Invokes all registered callbacks in registration order. No-op if called | ||
| // more than once. | ||
| void Cancel() { state_->Cancel(); } | ||
|
|
||
| // Returns true if Cancel() has been called. | ||
| bool IsCancelled() const { return state_->IsCancelled(); } | ||
|
|
||
| // Returns a token referring to this source's cancellation state. The | ||
| // token may be freely copied and passed to any number of | ||
| // CancellationCallbacks. | ||
| CancellationToken GetToken() const; | ||
|
|
||
| private: | ||
| std::shared_ptr<CancellationState> state_; | ||
| }; | ||
|
|
||
| // CancellationToken is the read end of a cancellation pair. Tokens are | ||
| // obtained from CancellationSource::GetToken() and passed to | ||
| // CancellationCallback constructors to register callbacks. | ||
| // | ||
| // A default-constructed token has no associated state: any | ||
| // CancellationCallback constructed from it is never invoked. | ||
| // | ||
| // CancellationToken is cheap to copy; all copies share the same underlying | ||
| // CancellationState. | ||
| class CancellationToken { | ||
| public: | ||
| CancellationToken() = default; | ||
|
|
||
| explicit CancellationToken(std::shared_ptr<CancellationState> state) | ||
| : state_(std::move(state)) {} | ||
|
|
||
| // Returns true if the associated source has been cancelled, or false if | ||
| // there is no associated source. | ||
| bool IsCancelled() const { return state_ && state_->IsCancelled(); } | ||
|
|
||
| private: | ||
| std::shared_ptr<CancellationState> state_; | ||
|
|
||
| friend class CancellationCallback; | ||
| }; | ||
|
|
||
| inline CancellationToken CancellationSource::GetToken() const { | ||
| return CancellationToken(state_); | ||
| } | ||
|
|
||
| // CancellationCallback registers a callback to be invoked when the associated | ||
| // CancellationSource is cancelled. The callback is invoked on whichever thread | ||
| // calls CancellationSource::Cancel(). | ||
| // | ||
| // The design follows C++20's std::stop_callback: | ||
| // | ||
| // - Constructing a CancellationCallback registers the callback. If the | ||
| // source was already cancelled, the callback is invoked immediately in the | ||
| // constructor. | ||
| // - Destroying a CancellationCallback deregisters the callback. If the | ||
| // callback is currently executing on another thread, the destructor blocks | ||
| // until execution completes, preventing use-after-free of anything captured | ||
| // by the callback. | ||
| // - CancellationCallback is non-copyable and non-movable, matching C++20's | ||
| // stop_callback. | ||
| class CancellationCallback { | ||
| public: | ||
| CancellationCallback(CancellationToken token, Continuation<void()> cb) | ||
| : state_(token.state_), | ||
| id_(state_ ? state_->Register(std::move(cb)) | ||
| : CancellationState::kAlreadyCancelled) {} | ||
|
|
||
| ~CancellationCallback() { | ||
| if (state_) { | ||
| state_->Deregister(id_); | ||
| } | ||
| } | ||
|
|
||
| CancellationCallback(CancellationCallback const&) = delete; | ||
| CancellationCallback& operator=(CancellationCallback const&) = delete; | ||
| CancellationCallback(CancellationCallback&&) = delete; | ||
| CancellationCallback& operator=(CancellationCallback&&) = delete; | ||
|
|
||
| private: | ||
| std::shared_ptr<CancellationState> state_; | ||
| CancellationState::CallbackId id_; | ||
| }; | ||
|
|
||
| } // namespace launchdarkly::async |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing move on
cancel_timercauses unnecessary copyLow Severity
cancel_timeris passed as an lvalue to theCancellationCallbackconstructor, causing the lambda (and its three captures:timer,executor,timer_started_future) to be copied rather than moved into theContinuation<void()>. Sincecancel_timeris never used after this line,std::move(cancel_timer)is the correct and idiomatic pattern here — consistent withstd::move(token)on the same line. This is also related to the PR discussion about the recursiveContinuationconstruction issue: while the SFINAE fix on theContinuationtemplate constructor is the proper fix, moving here avoids the extra copy path entirely.Reviewed by Cursor Bugbot for commit a187254. Configure here.