Update snippets/caching/memory-worker/CacheWorker.cs

[gurustron]

Summary

semaphore.Release will be called multiple times in the background worker loop, while it is acquired by it only once.

Another approach:

public sealed class CacheWorker : BackgroundService
{
    private readonly ILogger<CacheWorker> _logger;
    private readonly HttpClient _httpClient;
    private readonly CacheSignal<Photo> _cacheSignal;
    private readonly IMemoryCache _cache;
    private readonly TimeSpan _updateInterval = TimeSpan.FromHours(3);

    private const string Url = "https://jsonplaceholder.typicode.com/photos";

    public CacheWorker(
        ILogger<CacheWorker> logger,
        HttpClient httpClient,
        CacheSignal<Photo> cacheSignal,
        IMemoryCache cache) =>
        (_logger, _httpClient, _cacheSignal, _cache) = (logger, httpClient, cacheSignal, cache);

    public override async Task StartAsync(CancellationToken cancellationToken)
    {
        await _cacheSignal.WaitAsync();
        try
        {
            await FetchAndSetCache(cancellationToken);
        }
        finally
        {
            _cacheSignal.Release();   
        }
        await base.StartAsync(cancellationToken);
    }

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        while (!stoppingToken.IsCancellationRequested)
        {
            _logger.LogInformation("Updating cache.");

            await FetchAndSetCache(stoppingToken); 

            try
            {
                _logger.LogInformation(
                    "Will attempt to update the cache in {Hours} hours from now.",
                    _updateInterval.Hours);

                await Task.Delay(_updateInterval, stoppingToken);
            }
            catch (OperationCanceledException)
            {
                _logger.LogWarning("Cancellation acknowledged: shutting down.");
                break;
            }
        }
    }

    private async Task FetchAndSetCache(CancellationToken stoppingToken)
    {
        Photo[]? photos;
        try
        {
            photos = await _httpClient.GetFromJsonAsync<Photo[]>(
                    Url, stoppingToken);
        }
        catch (Exception e)
        {
            photos = new Photo[0];
        }

        if (photos is { Length: > 0 })
        {
            _cache.Set("Photos", photos);
            _logger.LogInformation(
                "Cache updated with {Count:#,#} photos.", photos.Length);
        }
        else
        {
            _logger.LogWarning(
                "Unable to fetch photos to update cache.");
        }
    }
}

[IEvangelist]

Hi @gurustron - thank you for this PR. While I agree that a change is needed, I disagree with the proposed changes. I think it's cleaner to call await _cacheSignal.WaitAsync(); just before the try.

I think we should do the following instead:

using System.Net.Http.Json;
using Microsoft.Extensions.Caching.Memory;

namespace CachingExamples.Memory;

public sealed class CacheWorker : BackgroundService
{
    private readonly ILogger<CacheWorker> _logger;
    private readonly HttpClient _httpClient;
    private readonly CacheSignal<Photo> _cacheSignal;
    private readonly IMemoryCache _cache;
    private readonly TimeSpan _updateInterval = TimeSpan.FromHours(3);

    private const string Url = "https://jsonplaceholder.typicode.com/photos";

    public CacheWorker(
        ILogger<CacheWorker> logger,
        HttpClient httpClient,
        CacheSignal<Photo> cacheSignal,
        IMemoryCache cache) =>
        (_logger, _httpClient, _cacheSignal, _cache) = (logger, httpClient, cacheSignal, cache);

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        while (!stoppingToken.IsCancellationRequested)
        {
            _logger.LogInformation("Updating cache.");

            await _cacheSignal.WaitAsync();

            try
            {
                Photo[]? photos =
                    await _httpClient.GetFromJsonAsync<Photo[]>(
                        Url, stoppingToken);

                if (photos is { Length: > 0 })
                {
                    _cache.Set("Photos", photos);
                    _logger.LogInformation(
                        "Cache updated with {Count:#,#} photos.", photos.Length);
                }
                else
                {
                    _logger.LogWarning(
                        "Unable to fetch photos to update cache.");
                }
            }
            finally
            {
                _cacheSignal.Release();
            }

            try
            {
                _logger.LogInformation(
                    "Will attempt to update the cache in {Hours} hours from now.",
                    _updateInterval.Hours);

                await Task.Delay(_updateInterval, stoppingToken);
            }
            catch (OperationCanceledException)
            {
                _logger.LogWarning("Cancellation acknowledged: shutting down.");
                break;
            }
        }
    }
}

In fact, in looking at this code. We could probably delete the ICacheSignal altogether, as it isn't needed.

[gurustron]

@IEvangelist as far as I understand the sole purpose of it was described in the article in the note:

You need to override BackgroundService.StartAsync and call await _cacheSignal.WaitAsync() in order to prevent a race condition between the starting of the CacheWorker and a call to PhotoService.GetPhotosAsync.

So it seems that it is needed in some way (though I would change it to be something like OneTimeSignal which will have bool flag in it) and your proposed code change does not handle this issue (though I have some doubts that current approach completely handles that too, but I will need to do some digging to be sure here).

Or this note should be scrapped also.

[IEvangelist]

Ah, I think through several refactorings things changed. That alert specifically calls out the PhotoService which doesn't even exist in the CacheWorker. I'll update this article, and fix this code. Thank you for calling attention to this issue.

[gurustron]

@IEvangelist isn't PhotoService some other service which consumes photos which will wait for cache to be populated and not used in the worker? See Program.cs and PhotoService.cs?

[IEvangelist]

@IEvangelist isn't PhotoService some other service which consumes photos which will wait for cache to be populated and not used in the worker? See Program.cs and PhotoService.cs?

Nice catch, yes, you're correct. I'm not sure how I missed that. I'll update this correctly in #33597.