Lazy (and AsyncLazy) don't handle exceptions very well

[mariusGundersen]

Lazy has 3 modes of operation, but it is missing the mode that would be most useful in LazyCache, namely supporting multi-threading and not caching exceptions. Since that mode is missing the current implementation will cache the exception thrown by the factory function and rethrow it for every access to the lazy. That's not a good thing when the Lazy itself is cached, as it means that every access of the cached value results in a thrown exception! More information about the problem is here: https://github.com/dotnet/corefx/issues/32337

There is a simple replacement for Lazy that can be used instead:

public class AtomicLazy<T>
{
    private readonly Func<T> _factory;

    private T _value;

    private bool _initialized;

    private object _lock;

    public AtomicLazy(Func<T> factory)
    {
        _factory = factory;
    }

    public T Value => LazyInitializer.EnsureInitialized(ref _value, ref _initialized, ref _lock, _factory);
}

This uses the same underlying LazyInitializer as Lazy, but it only supports one mode, the one that is missing (and, imo, most useful).

And for AsyncLazy:

public class AsyncLazy<T>
{
    private readonly Func<Task<T>> _factory;

    private Task<T> _task;

    private bool _initialized;

    private object _lock;

    public AsyncLazy(Func<Task<T>> factory)
    {
        _factory = factory;
    }

    public async Task<T> Value()
    {
        try
        {
            return await LazyInitializer.EnsureInitialized(ref _task, ref _initialized, ref _lock, _factory);
        }
        catch
        {
            Volatile.Write(ref _initialized, false);
            throw;
        }
    }
}

[alastairtree]

Hey, thanks for this - useful info! Although LazyCache will already trap an exception in the Lazy evaluation and ensure it does not get cached, (which is validated in this test ) what you present does seem like another way of handling this scenario.

[mariusGundersen]

With these Lazy implementations I don't think the GetOrAdd/GetorAddAsync methods need so much locking and try/catch. This also means that it doesn't have to lock on the entire cache, but instead it will create a lock per key.

[extremeandy]

I think there is potentially a concurrency issue in CachingService with the current method of removing faulted Lazy from the cache.

This is fine for removing it:

try
{
    return GetValueFromLazy<T>(cacheItem);
}
catch //addItemFactory errored so do not cache the exception
{
    CacheProvider.Remove(key);
    throw;
}

However, there is a possibility that before this code executes, another thread comes along and reads the faulted Lazy (e.g. with GetOrAdd) before this thread has a chance to remove it. This would result in the 2nd thread throwing the same exception, when really that 2nd thread should have had a chance to execute its factory.

I am not sure there is really any clean solution for apart from rolling your own keyed collection of locks, or perhaps Marius's solution would fix it?

I think one possible issue with the AtomicLazy solution is that the factory itself could somehow be in a broken state, and thus throw an Exception on every attempt to access Value. Would it be better to completely throw away the faulted Lazy and allow the next call to GetOrAdd to use the newly supplied factory?

[olegkv]

I've been using LazyCache for quite a while. It is great component; however, handling errors may be improved, I think. Here is what I'd propose, IMHO.

1.Current code has race condition. CacheProvider.Remove(key) can remove a valid cache entry if other threads already re-filled the cache for a given key after the error. Instead, removal of the key should take same lock per key (using call to CompareExchange), inside the lock verify that key still needs to be removed (other thread have not done it recently), remove the key and then exit lock. Using key lock in such way eliminates race condition so that valid cache entry cannot get removed by accident, but it does not guarantee that some other caller will not get same cached error, because it is easily possible that, for instance, two threads await same task from AsyncLazy and so that both threads would get same error, removing key from cache by one of them will not help the other one, because they both awaited same task. To solve this problem, second proposition is below.

2.It seems to me that some scenarios consider caching errors to be ok and some other scenarios do not accept that. For instance, caching network error for 5 seconds may be desirable behavior in some cases. And some other scenarios would require caching errors to never happen, it really seems dependent on particular task. Even more, it may depend on type of exception, for example, validation error is ok for caching, but network error is not. Also, if an error is not ok to be cached, caller thread would need some easy way to remove error from cache and re-try the call again without race condition and risk of removing valid cache entry mentioned above in point #1. So, I'd propose to make that choice in the calling thread, not in the LazyCache itself. To do that, it seems enough (correct me if I am wrong) to add a new method to LazyCache, something like: bool ClearError(string key, Exception exception) such method would -acquire same lock per key which LazyCache already uses (using CompareExchange call). -remove key if exactly same exception as the passed one is stored for the key -exit lock -return true if removal really happened and false if key did not store that exception.

That would allow multiple threads to make their own decisions about retrying and caching errors but without race conditions. Also, using separate method for clearing errors would allow caller threads to make decisions about caching errors using custom logic which is not possible if LazyCache does it on its own.