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

CachePolicy does not cache results of type default(TResult) #523

Closed
reisenberger opened this issue Oct 27, 2018 · 8 comments · Fixed by #581
Closed

CachePolicy does not cache results of type default(TResult) #523

reisenberger opened this issue Oct 27, 2018 · 8 comments · Fixed by #581

Comments

@reisenberger
Copy link
Member

CachePolicy does not cache results of type default(TResult)

value-type case

x-ref:

reference-type case

ie ability to cache null

x-ref:

@reisenberger
Copy link
Member Author

@cmeeren We are now working towards a major release (v7.0) of Polly, so we have the opportunity to take the breaking change to fix this. Are you interested in pitching in a PR to fix this?

It relates to the issue you raised in more detail here.


Needed would be:

(1) Adapt the ISyncCacheProvider and IAsyncCacheProvider interfaces, so that the get methods were of the form (say):

(bool, object) TryGetValue(String key);

and

Task<(bool, object)> TryGetValueAsync(String key, CancellationToken cancellationToken, bool continueOnCapturedContext);
  • where the bool return value represents cache hit or miss. (The .Net Standard 1.1 target would need to take a dependency on the nuget package System.ValueTuple.)
  • Other suggestions welcome: the goal is to remove default(TResult) representing cache miss.
  • An alternative to pulling in System.ValueTuple could be to define a mini struct representing the value and whether or not we got one, similar to Service Fabric's ConditionalValue<TResult>

(2) Adapt the sync cache policy engine and async cache policy engine for the fact that we would have some bool indicating cache hit/miss, rather than default(TResult)

(3) Add suitable/adapt existing tests


Then we would want to bring Polly's two cache provider implementations into line with these simple changes:

(a) Polly.Caching.MemoryCache
(b) Polly.Caching.IDistributedCache


We could also split more than one person over the work, if people were interested.

@reisenberger
Copy link
Member Author

Marking this 'up-for-grabs', for anyone interested

@cmeeren
Copy link
Contributor

cmeeren commented Dec 29, 2018

I'm currently swamped with other work, so for the sake of me and mine I have to pass on this. Hoping someone else will be able to do this! 🤞

@reisenberger
Copy link
Member Author

Thanks @cmeeren for the fast response. I also have to choose goals, so leaving this up-for-grabs ...

@cmeeren
Copy link
Contributor

cmeeren commented Jan 11, 2019

I'll try to take a stab at this today.

@cmeeren
Copy link
Contributor

cmeeren commented Jan 11, 2019

I think the common pattern is to use out params: bool TryGet(String key, out object result);

This also avoids an extra struct as well as the dependency on System.ValueTuple. Will try this.

Edit: Oh wait, you can't do that with async. I'll go for tuples instead, I think.

@reisenberger
Copy link
Member Author

@cmeeren Awesome. And: thank you!

I'm hoping to get v7 out early next week (the code is otherwise done; only finishing doco), so that would be perfect timing. Thanks for contributing!

@reisenberger
Copy link
Member Author

Fixes merged to the v7.0.0 branch

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

Successfully merging a pull request may close this issue.

2 participants