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

Fixes #285: InMemoryCacheClient ReplaceIfEqualAsync was not setting cache expiration causing locks to fail #287

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

niemyjski
Copy link
Member

@niemyjski niemyjski commented Jul 26, 2023

No changes are required for the Redis client (tests passed)

@niemyjski niemyjski added the bug label Jul 26, 2023
@niemyjski niemyjski self-assigned this Jul 26, 2023
@niemyjski niemyjski requested a review from ejsmith July 26, 2023 01:43
@@ -401,7 +401,10 @@ public InMemoryCacheClient(Builder<InMemoryCacheClientOptionsBuilder, InMemoryCa

collection.Add(stringValue);
cacheEntry.Value = collection;
cacheEntry.ExpiresAt = expiresAt;

if (expiresIn.HasValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is doing an add or update. Seems like we should be setting the expiresAt always. What is the point in making it conditionally set?

Copy link
Member Author

@niemyjski niemyjski Jul 26, 2023

Choose a reason for hiding this comment

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

Because if you pass in null for the expiresIn argument, we shouldn't update any existing expiresAt (local var computed to DateTime.MaxValue in a ternary operator).

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense for updating an existing entry, but if it's a new entry I think we need to set it to the default value.

Copy link
Member Author

@niemyjski niemyjski Jul 26, 2023

Choose a reason for hiding this comment

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

It's correct:

 var entry = new CacheEntry(items, expiresAt, _shouldClone);
                _memory.AddOrUpdate(key, entry, (k, cacheEntry) => {
                    if (!(cacheEntry.Value is ICollection<string> collection))
                        throw new InvalidOperationException($"Unable to add value for key: {key}. Cache value does not contain a set");

                    collection.Add(stringValue);
                    cacheEntry.Value = collection;

                    if (expiresIn.HasValue)
                        cacheEntry.ExpiresAt = expiresAt;

                    return cacheEntry;
                });

If it's an add the cache entry will use the expiresAt, if it's an update it will selectively update it.

@niemyjski niemyjski merged commit 0eea68f into master Jul 26, 2023
@niemyjski niemyjski deleted the bugfix/285-fix-in-memory-cache-client-expiration branch July 26, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants