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

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

Merged
merged 1 commit into from
Jul 26, 2023
+37 −38
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 8 additions & 5 deletions src/Foundatio.TestHarness/Caching/CacheClientTestsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,16 +483,19 @@ public virtual async Task CanReplaceIfEqual() {
using (cache) {
await cache.RemoveAllAsync();

Assert.True(await cache.AddAsync("replace-if-equal", "123"));
var result = await cache.GetAsync<string>("replace-if-equal");
const string cacheKey = "replace-if-equal";
Assert.True(await cache.AddAsync(cacheKey, "123"));
var result = await cache.GetAsync<string>(cacheKey);
Assert.NotNull(result);
Assert.Equal("123", result.Value);
Assert.Null(await cache.GetExpirationAsync(cacheKey));

Assert.False(await cache.ReplaceIfEqualAsync("replace-if-equal", "456", "789"));
Assert.True(await cache.ReplaceIfEqualAsync("replace-if-equal", "456", "123"));
result = await cache.GetAsync<string>("replace-if-equal");
Assert.False(await cache.ReplaceIfEqualAsync(cacheKey, "456", "789", TimeSpan.FromHours(1)));
Assert.True(await cache.ReplaceIfEqualAsync(cacheKey, "456", "123", TimeSpan.FromHours(1)));
result = await cache.GetAsync<string>(cacheKey);
Assert.NotNull(result);
Assert.Equal("456", result.Value);
Assert.NotNull(await cache.GetExpirationAsync(cacheKey));
}
}

Expand Down
32 changes: 22 additions & 10 deletions src/Foundatio/Caching/InMemoryCacheClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
Expand Down Expand Up @@ -401,7 +401,10 @@ public async Task<long> ListAddAsync<T>(string key, IEnumerable<T> values, TimeS

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 •
edited
Loading

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 •
edited
Loading

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.

cacheEntry.ExpiresAt = expiresAt;

return cacheEntry;
});

Expand All @@ -417,7 +420,10 @@ public async Task<long> ListAddAsync<T>(string key, IEnumerable<T> values, TimeS

collection.AddRange(items);
cacheEntry.Value = collection;
cacheEntry.ExpiresAt = expiresAt;

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

return cacheEntry;
});

Expand Down Expand Up @@ -452,7 +458,9 @@ public Task<long> ListRemoveAsync<T>(string key, IEnumerable<T> values, TimeSpan
cacheEntry.Value = collection;
}

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

if (_logger.IsEnabled(LogLevel.Trace)) _logger.LogTrace("Removed value from set with cache key: {Key}", key);
return cacheEntry;
});
Expand All @@ -468,7 +476,9 @@ public Task<long> ListRemoveAsync<T>(string key, IEnumerable<T> values, TimeSpan
cacheEntry.Value = collection;
}

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

if (_logger.IsEnabled(LogLevel.Trace)) _logger.LogTrace("Removed value from set with cache key: {Key}", key);
return cacheEntry;
});
Expand Down Expand Up @@ -563,18 +573,20 @@ public async Task<bool> ReplaceIfEqualAsync<T>(string key, T value, T expected,

var expiresAt = expiresIn.HasValue ? SystemClock.UtcNow.SafeAdd(expiresIn.Value) : DateTime.MaxValue;
bool wasExpectedValue = false;
bool success = _memory.TryUpdate(key, (k, e) => {
var currentValue = e.GetValue<T>();
bool success = _memory.TryUpdate(key, (k, cacheEntry) => {
var currentValue = cacheEntry.GetValue<T>();
if (currentValue.Equals(expected)) {
e.Value = value;
cacheEntry.Value = value;
wasExpectedValue = true;

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

return e;
return cacheEntry;
});

success = success && wasExpectedValue;

await StartMaintenanceAsync().AnyContext();

if (_logger.IsEnabled(LogLevel.Trace))
Expand Down
2 changes: 0 additions & 2 deletions tests/Foundatio.Tests/Caching/InMemoryCacheClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Foundatio.Caching;
using Foundatio.Utility;
Expand Down
3 changes: 1 addition & 2 deletions tests/Foundatio.Tests/Caching/InMemoryHybridCacheClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using System.Threading.Tasks;
using System.Threading.Tasks;
using Foundatio.Caching;
using Foundatio.Messaging;
using Microsoft.Extensions.Logging;
Expand Down
3 changes: 1 addition & 2 deletions tests/Foundatio.Tests/Serializer/JsonNetSerializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using Foundatio.Serializer;
using Foundatio.Serializer;
using Foundatio.TestHarness.Utility;
using Microsoft.Extensions.Logging;
using Xunit;
Expand Down
8 changes: 1 addition & 7 deletions tests/Foundatio.Tests/Serializer/MessagePackSerializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
using System;
using System.Collections.Generic;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Exporters.Json;
using BenchmarkDotNet.Loggers;
using BenchmarkDotNet.Reports;
using Foundatio.Serializer;
using Foundatio.Serializer;
using Foundatio.TestHarness.Utility;
using Microsoft.Extensions.Logging;
using Xunit;
Expand Down
3 changes: 1 addition & 2 deletions tests/Foundatio.Tests/Serializer/SystemTextJsonSerializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using Foundatio.Serializer;
using Foundatio.Serializer;
using Foundatio.TestHarness.Utility;
using Microsoft.Extensions.Logging;
using Xunit;
Expand Down
3 changes: 1 addition & 2 deletions tests/Foundatio.Tests/Serializer/Utf8JsonSerializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using Foundatio.Serializer;
using Foundatio.Serializer;
using Foundatio.TestHarness.Utility;
using Microsoft.Extensions.Logging;
using Xunit;
Expand Down
5 changes: 1 addition & 4 deletions tests/Foundatio.Tests/Utility/ConnectionStringParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
using System;
using System.Collections.Generic;
using Foundatio.Xunit;
using System;
using Xunit;
using Xunit.Abstractions;
using Foundatio.Utility;

namespace Foundatio.Tests.Utility {
Expand Down
3 changes: 1 addition & 2 deletions tests/Foundatio.Tests/Utility/RunTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System.Collections.Generic;
using Foundatio.Xunit;
using Foundatio.Xunit;
using Foundatio.Utility;
using Xunit;
using Xunit.Abstractions;
Expand Down