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.

Sign up for GitHub

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

Jump to bottom

Fix CachingService.TryGetValue returning wrong value #177

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix CachingService.TryGetValue returning wrong value #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac8c9da
Change TryGetValue Caching Service & Tests
phadtrapong May 7, 2022
9056dcf
Fix logic for normal cache add value
phadtrapong May 7, 2022
7be5ab8
Fix TryGetvalue logic
phadtrapong May 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions LazyCache.UnitTests/CachingServiceMemoryCacheProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1169,5 +1169,18 @@ public void TryGetReturnsCachedValueAndTrue()

Assert.Throws<InvalidCastException>(() => sut.TryGetValue<int>(key, out var value3));
}

[Test]
public void TryGetReturnsCachedValueFromGetOrAdd()
{
sut.GetOrAdd(TestKey, () => testObject);
var isSuccess = sut.TryGetValue<ComplexTestObject>(TestKey, out var actual);
var expected = testObject;

Assert.IsTrue(isSuccess);
Assert.IsNotNull(actual);
Assert.AreEqual(expected, actual);
}

}
}
11 changes: 10 additions & 1 deletion LazyCache/CachingService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,16 @@ public virtual bool TryGetValue<T>(string key, out T value)
{
ValidateKey(key);

return CacheProvider.TryGetValue(key, out value);
try
{
var contains = CacheProvider.TryGetValue<Lazy<T>>(key, out var lazyFactory);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use TryGetValue and then do something to coerce the value, instead of relying on catching cast exceptions? Also, as a larger issue, instead of storing both Lazy and non-Lazy values in CacheProvider, what if you just always stored a Lazy? That way you don't have to figure out what is in there when retrieving the values.

value = GetValueFromLazy<T>(lazyFactory, out var _);
return contains;
}
catch (InvalidCastException)
{
return CacheProvider.TryGetValue<T>(key, out value);
}
Comment on lines +95 to +104
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
try
{
var contains = CacheProvider.TryGetValue<Lazy<T>>(key, out var lazyFactory);
value = GetValueFromLazy<T>(lazyFactory, out var _);
return contains;
}
catch (InvalidCastException)
{
return CacheProvider.TryGetValue<T>(key, out value);
}
if (CacheProvider.TryGetValue(key, out object item))
{
value = GetValueFromLazy<T>(item, out _);
return true;
}
value = default;
return false;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaompneves - Given that CacheProvider can still contain multiple types, I think you have made the right change, here. I still think this whole implementation would be a lot cleaner if it only put Lazy in CacheProvider, so you knew to only expect Lazy when pulling values out. I'm not sure how to do that and continue to support AsyncLazy, though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your ideas, sound standard popular known best practices. I think with this specific scenario. It will introduce extra overhead Lazy types everywhere, which the current PR approach it also won't break many existing implement ation and clients. What do you think ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Coder3333 If you look at the GetValueFromLazy already handles the async lazy scenario in a sync way. In fact I just used a similar approach that the Getdoes.

}

public virtual T GetOrAdd<T>(string key, Func<ICacheEntry, T> addItemFactory)
Expand Down