alastairtree / LazyCache

An easy to use thread safe in-memory caching service with a simple developer friendly API for c#
https://nuget.org/packages/LazyCache
MIT License
1.71k stars 159 forks source link

SetAbsoluteExpirationFromRelative called before async Task execution #148

Closed theit8514 closed 3 years ago

theit8514 commented 3 years ago

Describe the bug I am using the latest LazyCache master branch and am having trouble with setting AbsoluteExpirationRelativeToNow.

Calling GetOrAddAsync with a long running task does not properly set AbsoluteExpiration. For the unit test added in #125 the AbsoluteExpirationRelativeToNow is set at the beginning of the task, so is likely to succeed. However, if addItemFactory is a longer running task (e.g. calling database or API) and then the AbsoluteExpirationRelativeToNow is set, the system will not set AbsoluteExpiration.

To Reproduce

        [Fact]
        public void GetOrAdd()
        {
            var key = "test";
            var expected = _cache.GetOrAdd(key, entry =>
            {
                Thread.Sleep(300);
                entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(5);
                return "Value";
            });
            var actual = _cache.Get<string>(key);
            Assert.NotNull(actual);
            Assert.Equal(expected, actual);

            Thread.Sleep(5500);

            // Will return null here, since GetOrAdd runs addItemFactory synchronously.
            var value = _cache.Get<string>(key);
            Assert.Null(value);
        }

        [Fact]
        public async Task GetOrAddAsync()
        {
            var key = "test";
            var expected = await _cache.GetOrAddAsync(key, async entry =>
            {
                await Task.Delay(300);
                entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(5);
                return "Value";
            });
            var actual = _cache.Get<string>(key);
            Assert.NotNull(actual);
            Assert.Equal(expected, actual);

            Thread.Sleep(5500);

            // Will return "Value" here, since GetOrAddAsync runs addItemFactory asynchronously and the task does not immediately set AbsoluteExpirationRelativeToNow.
            var value = _cache.Get<string>(key);
            Assert.Null(value);
        }

Expected behavior SetAbsoluteExpirationFromRelative should be called after addItemFactory is evaluated in GetOrAddAsync.

theit8514 commented 3 years ago

This can be solved by using ContinueWith on the task returned by addItemFactory and executing any post-initialization code required.

            object CacheFactory(ICacheEntry entry)
            {
                return new AsyncLazy<T>(() =>
                {
                    return addItemFactory(entry)
                        ?.ContinueWith(task =>
                        {
                            SetAbsoluteExpirationFromRelative(entry);
                            EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy<T>(entry.PostEvictionCallbacks);
                            return task.Result;
                        });
                });
            }