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

Fix CachingService.TryGetValue returning wrong value #177

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

Conversation

phadtrapong
Copy link

Changes
related with #176

  • Change TryGetValue to extract value from lazy and return correct value

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

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.

Comment on lines +95 to +104
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);
}

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;

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 ?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants