Proper IDisposable support in CachingService

[ghen]

CachingService class cerates ICacheProvider instance silently in constructor. The last one supports IDisposable.

Expected CachingService to be disposable (e.g. implement IDisposable) too in order to dispose internal resources properly.

[alastairtree]

Hmmmm. Good question. Yes technically you are correct - it can create an instance that should be disposed, but to be clear it creates exactly one global static instance CachingService.DefaultCacheProvider and does so only if you use the default constructor.

Really that constructor is there to maintain compatibility with older pre dot net core style users who are not using dependency injection - it mimics the old version's default constructor that referenced the global default cache instance of Runtime.Caching.MemoryCache. If you use Microsoft.Extensions.DependencyInjection, LazyCache.AspNetCore and the AddLazyCache() method in DI startup then that constructor is never used and so nothing would ever need to be disposed. Perhaps a better solution would be to obsolete that constructor and the default property? In normal usage it does not need to be disposed, and seems overkill to make all instances disposable for a single instance of the provider you may not be using.

[ghen]

Thanks for the explanation. Marking it as Obsolete sounds like a valid idea.

It just this logic was not so obvious from the constructor signature 😊.

Sincerely,

Eugene

From: Alastair Crabtree notifications@github.com Sent: Wednesday, 28 March 2018 10:21 AM To: alastairtree/LazyCache LazyCache@noreply.github.com Cc: Eugene yarshevich@gmail.com; Author author@noreply.github.com Subject: Re: [alastairtree/LazyCache] Proper IDisposable support in CachingService (#29)

Hmmmm. Good question. Yes technically you are correct - it can create an instance that should be disposed, but to be clear it creates exactly one global static instance CachingService.DefaultCacheProvider and does so only if you use the default constructor.

Really that constructor is there to maintain compatibility with older pre dot net core style users who are not using dependency injection - it mimics the old version's default constructor that referenced the global default cache instance of Runtime.Caching.MemoryCache. If you use Microsoft.Extensions.DependencyInjection, LazyCache.AspNetCore https://github.com/alastairtree/LazyCache/blob/feat/netcore2/LazyCache.AspNetCore/LazyCacheServiceCollectionExtensions.cs and the AddLazyCache() method in DI startup then that constructor is never used and so nothing would ever need to be disposed. Perhaps a better solution would be to obsolete that constructor and the default property? In normal usage it does not need to be disposed, and seems overkill to make all instances disposable for a single instance of the provider you may not be using.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alastairtree/LazyCache/issues/29#issuecomment-376707209 , or mute the thread https://github.com/notifications/unsubscribe-auth/AC7xDdm_0IlSpLxkTQfIMG5qMP3hP2z0ks5tislpgaJpZM4S6kcP .

[bradley-varol]

Does this have anything to do with the following exception that I'm getting when I hit the cache?: System.ObjectDisposedException: Cannot access a disposed object. A common cause of this error is disposing a context that was resolved from dependency injection and then later trying to use the same context instance elsewhere in your application. This may occur if you are calling Dispose() on the context, or wrapping the context in a using statement. If you are using dependency injection, you should let the dependency injection container take care of disposing context instances. Object name: 'DataContext'.

[alastairtree]

@bradley-varol - that is an error from Entity Framework, not lazy cache. Probably your cache delegate is running a query on a DataContext too late, after it has been disposed. Or maybe you need to await the async query before disposing the content.

[bradley-varol]

I don't fully understand your answer. The query is not asynchronous, and the exception is only thrown on the second use of the same key (accessing cache, not the delegate).

Here is some sudo code of what i'm doing (both _cache and _postsRepository are DI): var posts = _cache.GetOrAdd("Posts", () => _postsRepository.GetPosts());

If i do var posts = _postsRepository.GetPost(); I have no issues.

Edit: Okay having done some reading, it seems that this is because i'm not actually storing the DataContext query results to a list..?

[alastairtree]

Are you using lazy loading in EF? Probably you are accessing a property that is triggering a DB query on the second cache call which is after the context was disposed. if caching it is best not to use lazy loading for this reason.

[bradley-varol]

Okay, turned out to be that I was returning an EntityQueryable. Calling ToList() in my query fixed this. Sorry to have posted this here, and thank you for commenting and being helpful.

[alastairtree]

.Include() is eager loading, rather than lazy. Try adding AsNoTracking() to stop the entities holding a connection to the DbContext

[bradley-varol]

I'd like to Eager load these entities though. Thanks!

[alastairtree]

@ghen I have now fixed the creating of disposable provider in the default constructor by marking it obsolete as discussed. Will close this issue now.