DI Service with proper cache and dbContext usage

Question

I really want to make a nice, clean and proper code so I have few basic questions. At start I have a service with GetName. _dbContext is from other DI service like _cache

public Task<string> GetName(string title)
{
    var articleList = await _cache.GetOrCreate("CacheKey", entry =>
    {
        entry.SlidingExpiration = TimeSpan.FromSeconds(60 * 60);
        return _dbContext.Articles.ToListAsync;
    });

    var article = articleList.Where(c => c.Title == title).FirstOrDefault()

    if(article == null)
    {
        return "Non exist"
    }

    return article.Name();
}

1. In official docs we can read that EF context is not thread safe so I should add await after return in GetOrCreate?
2. Will it be appropriate to set it as a Singleton service and use it in actions and razor views?
3. In official docs we can read: avoid "data holder" objects that only exist to allow access to some other object in DI services. I am worrying about that line when I see my service.

Answer 1

The fact that EF context is not thread-safe is not related to async\await, so your first question does not make much sense. General best practice anyway is to not reuse single EF context for multiple logical operations. In multithreaded applications violation of this rule usually leads to disaster right away. If you use the same EF context from single thread at a time - violation might lead to certain unexpected behavior, especially if you use the same context for many operations for a long time.

In ASP.NET each request has dedicated thread, each request does not last for a long time, and each request usually represents single logical operation. For that reason, EF context is usually registered with scoped lifetime, which means each request has separate context which is disposed when request ends. For most uses this pattern works just fine (though you might still get unexpected behavior if you execute complex logic with many database requests in single http request). I personally still prefer to use new instance of EF context for every operation (so in your example I would have injected EF context factory and create new instance inside GetOrCreate body, which then immediately dispose). However, this is not a very popular opinion.

So, each request has separate context (if it's registered with scoped lifetime) so you should not (in general, if you don't spawn background threads yourself) bother about multithreaded access to that instance.

Your method though is still incorrect, because what you now save in your cache is not List<Article>. You store Task<List<Acticle>> instead. With memory cache it works, because storing in memory cache does not involve any serialization. But as soon as you change your cache provider - things will break, because Task is obviously not serializable. Instead use GetOrCreateAsync (note there is still no reason to add await after return):

var articleList = await _cache.GetOrCreateAsync("CacheKey", entry =>
{
    entry.SlidingExpiration = TimeSpan.FromSeconds(60 * 60);
    return _dbContext.Articles.ToListAsync();
});

Note that if item is already present in cache - there will be no async work done, whole operation will be executed completely synchronously (don't let await fool you - if you await something it does not necessary mean there will be some background threads or async operations involved).

It's generally not good to store EF entities directly in cache. It's better to convert them to DTO entities and store those. However, if you store EF entities - at least don't forget to disable lazy-loading and proxy creation for your context.

It will not be appropriate to register such service as singleton because it depends on EF context which has scoped lifetime (by default). Register it with the same lifetime you have for EF context.

As for avoid "data holder" objects - I don't see how it's related here. Your service has a logic (get data from cache or database) - it does not exists only to allow access to some other objects.

Answer 2

In official docs we can read that EF context is not thread safe so I should add await after return in GetOrCreate?

It's not a matter of EF context not being thread-safe. When you call async methods, you should add await

public Task<string> GetName(string title)
{
    var articleList = await _cache.GetOrCreateAsync("CacheKey", entry =>
    {
        entry.SlidingExpiration = TimeSpan.FromSeconds(60 * 60);
        return await _dbContext.Articles.ToListAsync();
    });

    var article = await articleList.Where(c => c.Title == title).FirstOrDefaultAsync();

    if(article == null)
    {
        return "Non exist"
    }

    return article.Name();
}

Will it be appropriate to set it as a Singleton service and use it in actions and razor views?

I don't think it would be appropriate to set your service as a Singleton because your service depends on _dbContext which has a Scoped service lifetime. If you register your service as a Singleton, then you should only inject a Singleton (and _dbContext is obviously not Singleton... its Scoped). It is probably better to set your service with a Transient service lifetime. Note the warning from the asp.net core docs: "The main danger to be wary of is resolving a Scoped service from a singleton. It's likely in such a case that the service will have incorrect state when processing subsequent requests."

public void ConfigureServices(IServiceCollection services)
{
    // ...

    services.AddTransient<MyService>();

    services.AddDbContext<MyDbContext>(ServiceLifetime.Scoped);

    // ...
}

In official docs we can read: avoid "data holder" objects that only exist to allow access to some other object in DI services. I am worrying about that line when I see my service.

I can understand your worry. When I see GetName, I don't see why you can't just get the first article name from a list of articles from within your Controller classes. But then again, this is just a recommendation and shouldn't be taken as a hard rule.