The CacheSignal<T> class does not work as described in the article due to its usage of SemaphoreSlim
- Dominant language
- No language data
- Stars
- 4.8k
- Forks
- 6.1k
- Avg merge
- 15h 21m
- Merged PRs (30d)
- 370
Description
### Type of issue
Other (describe below)
### Description
The class `CacheSignal` should signal that the cache is populated before accessing it.
> Calls and waits for the _cacheSignal.WaitAsync() to release, this ensures that the cache is populated before accessing the cache.
The semaphore of the CacheSignal is already released in its initial state, it should be created with `new (0, 1)` instead of `new (1, 1)` otherwise the `PhotoService` may access an unpopulated cache.
```csharp
namespace CachingExamples.Memory;
public sealed class CacheSignal
{
private readonly SemaphoreSlim _semaphore = new(1, 1);
///
/// Exposes a that represents the asynchronous wait operation.
/// When signaled (consumer calls ), the
/// is set as .
///
public Task WaitAsync() => _semaphore.WaitAsync();
///
/// Exposes the ability to signal the release of the 's operation.
/// Callers who were waiting, will be able to continue.
///
public void Release() => _semaphore.Release();
}
```
**Suggestion**
Additionally I would suggest a refactor of the `CacheSignal` as it should only be released from the `CacheWorker` once the cache is populated. The `PhotoService` should not need that mutual access to the cache.
`CacheSignal` should utilize a `TaskCompletionSource` instead of a `SemaphoreSlim`.
```csharp
namespace CachingExamples.Memory;
public sealed class CacheSignal
{
private readonly TaskCompletionSource _tcs = new();
///
/// Exposes a that represents the asynchronous wait operation for the population of the cache.
/// When signaled (consumer calls ), the
/// is set as .
///
public Task WaitForPopulateAsync() => _tcs.Task;
///
/// Exposes the ability to signal the release of the 's operation to indicate that the cache has been populated.
/// This only needs to be called once after population of the cache.
/// Callers who were waiting, will be able to continue.
///
public void Release() => _tcs.TrySetResult();
}
```
The `PhotoService` should just wait for the population of the cache and not release it anymore. The cache itself is thread safe, therefore there is no need for the mutual access due to the previous behavior of the `CacheSignal`.
```csharp
using Microsoft.Extensions.Caching.Memory;
namespace CachingExamples.Memory;
public sealed class PhotoService(
IMemoryCache cache,
CacheSignal cacheSignal,
ILogger logger)
{
public async IAsyncEnumerable GetPhotosAsync(Func? filter = default)
{
await cacheSignal.WaitForPopulateAsync(); // wait until the cache is populated
Photo[] photos =
(await cache.GetOrCreateAsync(
"Photos", _ =>
{
logger.LogWarning("This should never happen!");
return Task.FromResult(Array.Empty());
}))!;
// If no filter is provided, use a pass-thru.
filter ??= _ => true;
foreach (Photo photo in photos)
{
if (!default(Photo).Equals(photo) && filter(photo))
{
yield return photo;
}
}
}
}
```
### Page URL
https://learn.microsoft.com/en-us/dotnet/core/extensions/caching
### Content source URL
https://github.com/dotnet/docs/blob/main/docs/core/extensions/caching.md
### Document Version Independent Id
07beacc6-ddcf-2fd5-b896-00f999764b00
### Platform Id
97e770aa-0b26-d69d-80e4-578056090645
### Article author
@IEvangelist
### Metadata
* ID: ed120c7a-f4c0-c1d1-cb8c-168a3eba6dd9
* PlatformId: 97e770aa-0b26-d69d-80e4-578056090645
* Service: **dotnet-fundamentals**
[Related Issues](https://github.com/dotnet/docs/issues?q=is%3Aissue+is%3Aopen+07beacc6-ddcf-2fd5-b896-00f999764b00)
Contributor guide
Assessment
This issue has not been assessed yet.