Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SetAbsoluteExpirationFromRelative called before async Task execution #148

Closed
theit8514 opened this issue Feb 8, 2021 · 1 comment · Fixed by #160
Closed

SetAbsoluteExpirationFromRelative called before async Task execution #148

theit8514 opened this issue Feb 8, 2021 · 1 comment · Fixed by #160
Labels

Comments

@theit8514
Copy link

theit8514 commented Feb 8, 2021

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 theit8514 added the bug label Feb 8, 2021
@theit8514
Copy link
Author

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;
                        });
                });
            }

allanrodriguez added a commit to allanrodriguez/LazyCache that referenced this issue Jul 24, 2021
…ative

allows AbsoluteExpiration to be set properly when factories are long-running and a relative expiration is specified

fixes alastairtree#148
alastairtree pushed a commit that referenced this issue Sep 1, 2021
…ExpirationRelativeToNow (#160)

* fix: await addItemFactory before calling SetAbsoluteExpirationFromRelative

allows AbsoluteExpiration to be set properly when factories are long-running and a relative expiration is specified

fixes #148

* test: verify that AbsoluteExpiration is set properly from a relative expiration when a factory is long-running

* test: improve unit test names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant