Tests for RemoveAllAsync + some tweaks to cache maintenance. #377
Tests for RemoveAllAsync + some tweaks to cache maintenance. #377
Conversation
| await cache.SetAsync("flag", true); | ||
| Assert.Equal(13422, (await cache.GetAsync<int>("test")).Value); | ||
| Assert.Null(await cache.GetAsync<int>("test2")); | ||
| Assert.False((await cache.GetAsync<int>("test2")).HasValue); |
There was a problem hiding this comment.
These tests were skipped so I think this is just something that was broken as GetAsync always returns a cache value and never null
| await CompactAsync().AnyContext(); | ||
|
|
||
| if (TimeSpan.FromMilliseconds(100) < utcNow - _lastMaintenance) | ||
| if (TimeSpan.FromMilliseconds(250) < utcNow - _lastMaintenance) |
There was a problem hiding this comment.
Bumped the time a bit, feels like we could even raise it higher, more of an issue with high operation count, this was really up in the performance profiler... There are several issues already with this method like not making sure it's already running, race conditions with setting _lastMaintenance.
| var expiresAt = kvp.Value.ExpiresAt; | ||
| if (expiresAt < DateTime.MaxValue && expiresAt <= utcNow) |
There was a problem hiding this comment.
Just trying to cut down a tiny bit on number of times expires is called.
There was a problem hiding this comment.
Pull Request Overview
This PR expands testing for cache removal functionality and refines cache maintenance behavior.
- Enabled tests for RemoveAll and RemoveAllKeys methods across multiple test files.
- Adjusted maintenance timing and compact logic in InMemoryCacheClient to conditionally compact the cache.
- Enhanced error logging by appending exception messages.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Foundatio.Tests/Caching/InMemoryHybridCacheClientTests.cs | Added tests for CanRemoveAllAsync and CanRemoveAllKeysAsync. |
| tests/Foundatio.Tests/Caching/InMemoryCacheClientTests.cs | Added similar removal tests and updated comments. |
| src/Foundatio/Caching/InMemoryCacheClient.cs | Tweaked maintenance conditions and logging enhancements. |
| src/Foundatio.TestHarness/Caching/HybridCacheClientTests.cs | Added removal tests for hybrid cache functionality. |
| src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs | Added implementations for RemoveAll and RemoveAllKeys tests, and adjusted assertions. |
Comments suppressed due to low confidence (2)
src/Foundatio/Caching/InMemoryCacheClient.cs:1021
- Ensure that gating the expiration removal solely on infrequent access does not bypass removal for frequently accessed but expired items.
if (!lastAccessTimeIsInfrequent) continue;
src/Foundatio/Caching/InMemoryCacheClient.cs:960
- Please confirm that increasing the maintenance threshold from 100ms to 250ms aligns with performance expectations, as this change may delay the removal of expired entries.
if (TimeSpan.FromMilliseconds(250) < utcNow - _lastMaintenance)
No description provided.