Add TryGetTokenForDataLossDetection method to avoid unnecessary CancellationTokenSource creation and Timer creation on every RPC #689
Conversation
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
…llationTokenSource creation - Added TryGetTokenForDataLossDetection() method to IWatchForRedisLosingAllItsData interface - Implemented the method in WatchForRedisLosingAllItsData to return token immediately if monitoring is active - Updated RedisPendingRequestQueue to use try method first, falling back to GetTokenForDataLossDetection - Refactored DequeueAsync to use centralized DataLossCancellationToken method instead of duplicating logic - Updated test implementations (RedisNeverLosesData and CancellableDataLossWatchForRedisLosingAllItsData) - Improves performance by avoiding CancellationTokenSource allocation when monitoring is already ready - Maintains backward compatibility with existing GetTokenForDataLossDetection method - Reduces code duplication by centralizing data loss cancellation token logic Resolves TODO comment in WatchForRedisLosingAllItsData.cs
LukeButters
force-pushed
the
feature/add-try-get-token-for-data-loss-detection
branch
from
f412257 to
df39b37
Compare
LukeButters
commented
Sep 18, 2025
Comment on lines
+121
to
+125
| var token = watchForRedisLosingAllItsData.TryGetTokenForDataLossDetection(); | ||
| if (token.HasValue) | ||
| { | ||
| return token.Value; | ||
| } |
There was a problem hiding this comment.
This new code is about 10x faster than the below code.
The below code is almost always never executed.
rhysparry
approved these changes
Sep 18, 2025
| return await localCopyOfTaskCompletionSource.Task.WaitAsync(cts.Token); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: extraneous blank line
|
|
||
| public CancellationToken? TryGetTokenForDataLossDetection() | ||
| { | ||
| if (TaskCompletionSource.Task.IsCompleted && TaskCompletionSource.Task.Status == TaskStatus.RanToCompletion) |
There was a problem hiding this comment.
nit: Not sure how I missed this before, but is there a reason TaskCompletionSource is a public field? Should we keep that one private if nothing is relying on accessing it directly?
There was a problem hiding this comment.
Wait it is private:
I guess I can remove the private and fall back to halibut defaults that make it private.
LukeButters
deleted the
feature/add-try-get-token-for-data-loss-detection
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Background
This is a simple change to reduce the amount of work we do. In most cases we will already have a Data Loss CT which means we can avoid creating multiple CTS and delayed tasks that will immediately throw.
Additionally the methods are updated such that we work on a consistent TaskCompletionSource since that value is replaced.
We are aiming to reduce as much work as possible in the normal case, to reduce queue overhead.
The AI wrote this:
How to review this PR
Quality ✔️
Pre-requisites