Repository navigation
[GreenDonut] Fix single-key LoadAsync caching a promise for a cancelled token - #149
Merged
Merged
Conversation
…ync touches the cache
…ngle-key LoadAsync
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the single-key cancellation-handling change.
No open issues 🎉
What changed in this PR
Fixes GreenDonut’s single-key LoadAsync path so an already-cancelled token throws before accessing the cache, matching the many-key overload.
Changes:
- Adds an early cancellation check to prevent caching a promise for an already-cancelled request.
- Adds a regression test verifying that cancellation throws
OperationCanceledExceptionand leaves the cache empty.
| File | Description |
|---|---|
src/GreenDonut/test/GreenDonut.Tests/DataLoaderTests.cs |
Tests cancellation handling and an unchanged empty cache. |
src/GreenDonut/src/GreenDonut/DataLoaderBase.cs |
Checks cancellation before single-key cache access. |
Review effort: Balanced
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The single-key
DataLoaderBase.LoadAsync(key, ct)now checks the token before it touches the cache, like the many-key overload already does. Before, a call with an already-cancelled token still added a promise to the cache, and the batch cancelled it afterwards.LoadAsync_Should_ThrowAndLeaveCacheEmpty_When_TokenIsCancelledchecks that the call throwsOperationCanceledExceptionand that the cache stays empty. It fails without the fix.GreenDonut.Tests passes on net10 and net8, and GreenDonut.Data.Tests passes on net10. The Docker-backed EF tests were not run. No benchmark was run. The change adds one token check per call.