Copilot commented on code in PR #7417:
URL: https://github.com/apache/ignite-3/pull/7417#discussion_r2694097567
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite/IgniteDistributedCache.cs:
##########
@@ -199,4 +258,35 @@ private async Task<IKeyValueView<string, CacheEntry>>
GetViewAsync()
_initLock.Release();
}
}
+
+ [SuppressMessage("Design", "CA1031:Do not catch general exception types",
Justification = "Background loop.")]
+ private async Task CleanupLoopAsync()
+ {
+ while (!_cleanupCts.Token.IsCancellationRequested)
+ {
+ try
+ {
+ await Task.Delay(_options.ExpiredItemsCleanupInterval,
_cleanupCts.Token).ConfigureAwait(false);
+ await
CleanupExpiredEntriesAsync(_cleanupCts.Token).ConfigureAwait(false);
+ }
+ catch (OperationCanceledException)
+ {
+ break;
+ }
+ catch (Exception)
+ {
+ // Swallow exceptions - might be intermittent connection
errors.
+ // Client will log the error and retry on the next iteration.
Review Comment:
The comment states "Client will log the error and retry on the next
iteration" but there is no explicit logging in the catch block. While the
underlying Ignite client may log errors internally, the comment is misleading
about where the logging occurs. Consider either adding explicit logging here or
updating the comment to clarify that logging happens at a lower level.
```suggestion
// Swallow exceptions - might be intermittent connection
errors; loop will retry on the next iteration.
```
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite/IgniteDistributedCache.cs:
##########
@@ -77,6 +73,16 @@ public IgniteDistributedCache(
_options = options;
_cacheEntryMapper = new CacheEntryMapper(options);
_igniteClientGroup = igniteClientGroup;
+
+ if (_options.ExpiredItemsCleanupInterval != Timeout.InfiniteTimeSpan)
+ {
+ if (_options.ExpiredItemsCleanupInterval <= TimeSpan.Zero)
+ {
+ throw new ArgumentException("ExpiredItemsCleanupInterval must
be positive or Timeout.InfiniteTimeSpan.", nameof(options));
+ }
+
+ _ = CleanupLoopAsync();
Review Comment:
The background cleanup task started in CleanupLoopAsync is not properly
awaited, which may lead to unobserved task exceptions. The task is discarded
with the discard operator (_), but if an exception occurs after the Dispose
method is called, it won't be properly handled. Consider storing the Task
reference and awaiting it during disposal, or use a more robust pattern for
background task management.
```suggestion
_ = CleanupLoopAsync().ContinueWith(
t =>
{
// Observe exception to avoid unobserved task exceptions
for the background cleanup loop.
var _ = t.Exception;
},
System.Threading.Tasks.TaskContinuationOptions.OnlyOnFaulted);
```
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite.Tests/IgniteDistributedCacheTests.cs:
##########
@@ -225,21 +231,112 @@ public async Task TestKeyPrefix()
}
[Test]
- public void TestExpirationNotSupported()
+ public async Task TestAbsoluteExpiration()
+ {
+ IDistributedCache cache = GetCache();
+
+ var entryOptions = new DistributedCacheEntryOptions
+ {
+ AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(0.5)
+ };
+
+ await cache.SetAsync("x", [1], entryOptions);
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ await Task.Delay(TimeSpan.FromSeconds(0.7));
+
+ Assert.IsNull(await cache.GetAsync("x"));
+ }
+
+ [Test]
+ public async Task TestAbsoluteExpirationRelativeToNow()
+ {
+ IDistributedCache cache = GetCache();
+
+ var entryOptions = new DistributedCacheEntryOptions
+ {
+ AbsoluteExpiration = DateTimeOffset.UtcNow.AddSeconds(0.5)
+ };
+
+ await cache.SetAsync("x", [1], entryOptions);
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ await Task.Delay(TimeSpan.FromSeconds(0.7));
+
+ Assert.IsNull(await cache.GetAsync("x"));
+ }
+
+ [Test]
+ public async Task TestSlidingExpiration()
{
- var cache = GetCache();
+ IDistributedCache cache = GetCache();
- Test(new() { AbsoluteExpiration = DateTimeOffset.Now });
- Test(new() { SlidingExpiration = TimeSpan.FromMinutes(1) });
- Test(new() { AbsoluteExpirationRelativeToNow = TimeSpan.FromHours(1)
});
+ var entryOptions = new DistributedCacheEntryOptions
+ {
+ SlidingExpiration = TimeSpan.FromSeconds(0.5)
+ };
+
+ await cache.SetAsync("x", [1], entryOptions);
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ // Access before expiration to reset the timer.
+ await Task.Delay(TimeSpan.FromSeconds(0.3));
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ // Wait less than the sliding window - should still be available.
+ await Task.Delay(TimeSpan.FromSeconds(0.3));
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ // Wait for expiration without accessing.
+ await Task.Delay(TimeSpan.FromSeconds(0.7));
+ Assert.IsNull(await cache.GetAsync("x"));
+ }
- void Test(DistributedCacheEntryOptions options)
+ [Test]
+ public async Task TestExpiredItemsCleanup()
+ {
+ var cacheOptions = new IgniteDistributedCacheOptions
+ {
+ ExpiredItemsCleanupInterval = TimeSpan.FromSeconds(1),
+ TableName = nameof(TestExpiredItemsCleanup),
+ CacheKeyPrefix = keyPrefix
+ };
+
+ IDistributedCache cache = GetCache(cacheOptions);
+
+ var entryOptions = new DistributedCacheEntryOptions
{
- var ex = Assert.Throws<ArgumentException>(() => cache.Set("x",
[1], options));
- Assert.AreEqual("Expiration is not supported. (Parameter
'options')", ex.Message);
- }
+ AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(0.5)
+ };
+
+ // Set multiple items with expiration.
+ await cache.SetAsync("x1", [1], entryOptions);
+ await cache.SetAsync("x2", [2], entryOptions);
+
+ Assert.IsNotNull(await cache.GetAsync("x1"));
+ Assert.IsNotNull(await cache.GetAsync("x2"));
+
+ // Wait for expiration.
+ await Task.Delay(TimeSpan.FromSeconds(0.7));
+
+ // Check cache.
+ Assert.IsNull(await cache.GetAsync("x1"));
+ Assert.IsNull(await cache.GetAsync("x2"));
+
+ // Check the underlying table.
+ await TestUtils.WaitForConditionAsync(
+ async () =>
+ {
+ var sql = $"SELECT * FROM {cacheOptions.TableName}";
+ await using var resultSet = await
Client.Sql.ExecuteAsync(null, sql);
+ return await resultSet.CountAsync() == 0;
+ },
+ timeoutMs: 3000,
+ messageFactory: () => "Expired items should be cleaned up from the
table");
Review Comment:
The cache instance created in this test is not disposed. Since this test
enables the cleanup task (ExpiredItemsCleanupInterval =
TimeSpan.FromSeconds(1)), the background cleanup loop will continue running
after the test completes. Consider disposing the cache at the end of the test
to properly clean up resources.
```suggestion
messageFactory: () => "Expired items should be cleaned up from
the table");
if (cache is System.IAsyncDisposable asyncDisposable)
{
await asyncDisposable.DisposeAsync();
}
else if (cache is System.IDisposable disposable)
{
disposable.Dispose();
}
```
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite/IgniteDistributedCache.cs:
##########
@@ -199,4 +258,35 @@ private async Task<IKeyValueView<string, CacheEntry>>
GetViewAsync()
_initLock.Release();
}
}
+
+ [SuppressMessage("Design", "CA1031:Do not catch general exception types",
Justification = "Background loop.")]
+ private async Task CleanupLoopAsync()
+ {
+ while (!_cleanupCts.Token.IsCancellationRequested)
+ {
+ try
+ {
+ await Task.Delay(_options.ExpiredItemsCleanupInterval,
_cleanupCts.Token).ConfigureAwait(false);
+ await
CleanupExpiredEntriesAsync(_cleanupCts.Token).ConfigureAwait(false);
+ }
+ catch (OperationCanceledException)
+ {
+ break;
+ }
+ catch (Exception)
+ {
+ // Swallow exceptions - might be intermittent connection
errors.
+ // Client will log the error and retry on the next iteration.
+ }
+ }
+ }
+
+ private async Task CleanupExpiredEntriesAsync(CancellationToken token)
+ {
+ var expireAtCol = _options.ExpirationColumnName;
+ var sql = $"DELETE FROM {_options.TableName} WHERE {expireAtCol} IS
NOT NULL AND {expireAtCol} <= ?";
Review Comment:
The SQL statement concatenates table and column names directly from
configuration options without any validation or sanitization. While the code
comment at line 238 acknowledges this assumption, this could lead to SQL
injection vulnerabilities if user-supplied values are used for TableName or
ExpirationColumnName. Consider adding validation to ensure these values contain
only alphanumeric characters and underscores, or use a different approach to
construct the SQL safely.
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite/IgniteDistributedCache.cs:
##########
@@ -77,6 +73,16 @@ public IgniteDistributedCache(
_options = options;
_cacheEntryMapper = new CacheEntryMapper(options);
_igniteClientGroup = igniteClientGroup;
+
+ if (_options.ExpiredItemsCleanupInterval != Timeout.InfiniteTimeSpan)
+ {
+ if (_options.ExpiredItemsCleanupInterval <= TimeSpan.Zero)
+ {
+ throw new ArgumentException("ExpiredItemsCleanupInterval must
be positive or Timeout.InfiniteTimeSpan.", nameof(options));
+ }
Review Comment:
There is no test coverage for the validation logic that throws
ArgumentException when ExpiredItemsCleanupInterval is less than or equal to
TimeSpan.Zero (excluding Timeout.InfiniteTimeSpan). Consider adding a test to
verify that invalid values are properly rejected.
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite.Tests/IgniteDistributedCacheTests.cs:
##########
@@ -225,21 +231,112 @@ public async Task TestKeyPrefix()
}
[Test]
- public void TestExpirationNotSupported()
+ public async Task TestAbsoluteExpiration()
+ {
+ IDistributedCache cache = GetCache();
+
+ var entryOptions = new DistributedCacheEntryOptions
+ {
+ AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(0.5)
+ };
+
+ await cache.SetAsync("x", [1], entryOptions);
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ await Task.Delay(TimeSpan.FromSeconds(0.7));
+
+ Assert.IsNull(await cache.GetAsync("x"));
+ }
+
+ [Test]
+ public async Task TestAbsoluteExpirationRelativeToNow()
+ {
+ IDistributedCache cache = GetCache();
+
+ var entryOptions = new DistributedCacheEntryOptions
+ {
+ AbsoluteExpiration = DateTimeOffset.UtcNow.AddSeconds(0.5)
+ };
+
+ await cache.SetAsync("x", [1], entryOptions);
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ await Task.Delay(TimeSpan.FromSeconds(0.7));
+
+ Assert.IsNull(await cache.GetAsync("x"));
+ }
Review Comment:
The test name "TestAbsoluteExpirationRelativeToNow" is misleading as it
actually tests the AbsoluteExpiration property (not
AbsoluteExpirationRelativeToNow). Consider renaming this test to
"TestAbsoluteExpiration" for clarity.
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite.Tests/IgniteDistributedCacheTests.cs:
##########
@@ -225,21 +231,112 @@ public async Task TestKeyPrefix()
}
[Test]
- public void TestExpirationNotSupported()
+ public async Task TestAbsoluteExpiration()
+ {
+ IDistributedCache cache = GetCache();
+
+ var entryOptions = new DistributedCacheEntryOptions
+ {
+ AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(0.5)
+ };
+
+ await cache.SetAsync("x", [1], entryOptions);
+ Assert.IsNotNull(await cache.GetAsync("x"));
+
+ await Task.Delay(TimeSpan.FromSeconds(0.7));
+
+ Assert.IsNull(await cache.GetAsync("x"));
+ }
Review Comment:
The test name "TestAbsoluteExpiration" is misleading as it actually tests
AbsoluteExpirationRelativeToNow property. Consider renaming this test to
"TestAbsoluteExpirationRelativeToNow" for clarity.
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite/IgniteDistributedCache.cs:
##########
@@ -151,7 +174,43 @@ public async Task RemoveAsync(string key,
CancellationToken token)
/// <inheritdoc/>
public void Dispose()
{
+ if (_cleanupCts.IsCancellationRequested)
+ {
+ return;
+ }
+
+ _cleanupCts.Cancel();
_initLock.Dispose();
+ _cleanupCts.Dispose();
+ }
Review Comment:
The Dispose method checks if cancellation is already requested and returns
early, which means subsequent calls to Dispose won't properly dispose of
_initLock and _cleanupCts. While Dispose should be idempotent, the current
implementation may skip necessary cleanup on subsequent calls. Consider using a
dedicated disposal flag or restructuring the logic to ensure all disposable
resources are properly cleaned up on every call.
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite.Tests/IgniteDistributedCacheTests.cs:
##########
@@ -176,7 +178,7 @@ public async Task TestNonExistingTable()
await Client.Sql.ExecuteAsync(null, $"DROP TABLE IF EXISTS
{tableName}");
- IDistributedCache cache = GetCache(new() { TableName = tableName });
+ IDistributedCache cache = GetCache(new() { TableName = tableName,
CacheKeyPrefix = keyPrefix});
Review Comment:
Missing space before the closing brace. Should be "keyPrefix }" instead of
"keyPrefix}".
```suggestion
IDistributedCache cache = GetCache(new() { TableName = tableName,
CacheKeyPrefix = keyPrefix });
```
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite/IgniteDistributedCache.cs:
##########
@@ -105,34 +132,30 @@ public async Task SetAsync(string key, byte[] value,
DistributedCacheEntryOption
ArgumentNullException.ThrowIfNull(value);
ArgumentNullException.ThrowIfNull(options);
- if (options.AbsoluteExpiration != null || options.SlidingExpiration !=
null || options.AbsoluteExpirationRelativeToNow != null)
- {
- // TODO: IGNITE-23973 Add expiration support
- throw new ArgumentException("Expiration is not supported.",
nameof(options));
- }
-
- var entry = new CacheEntry(value);
+ (long? expiresAt, long? sliding) = GetExpiration(options);
+ var entry = new CacheEntry(value, expiresAt, sliding);
IKeyValueView<string, CacheEntry> view = await
GetViewAsync().ConfigureAwait(false);
-
await view.PutAsync(transaction: null, key,
entry).ConfigureAwait(false);
}
/// <inheritdoc/>
- public void Refresh(string key)
- {
- ArgumentNullException.ThrowIfNull(key);
-
- // TODO: IGNITE-23973 Add expiration support
- }
+ public void Refresh(string key) =>
+ RefreshAsync(key, CancellationToken.None).GetAwaiter().GetResult();
/// <inheritdoc/>
- public Task RefreshAsync(string key, CancellationToken token)
+ public async Task RefreshAsync(string key, CancellationToken token)
{
ArgumentNullException.ThrowIfNull(key);
- // TODO: IGNITE-23973 Add expiration support
- return Task.CompletedTask;
+ IIgnite ignite = await
_igniteClientGroup.GetIgniteAsync().ConfigureAwait(false);
+
+ var sql = $"UPDATE {_options.TableName} " +
+ $"SET {_options.ExpirationColumnName} =
{_options.SlidingExpirationColumnName} + ? " +
+ $"WHERE {_options.KeyColumnName} = ? AND
{_options.SlidingExpirationColumnName} IS NOT NULL";
+
+ var actualKey = _options.CacheKeyPrefix + key;
+ await ignite.Sql.ExecuteAsync(null, sql, token, UtcNowMillis(),
actualKey).ConfigureAwait(false);
}
Review Comment:
There is no test coverage for the Refresh and RefreshAsync methods. These
methods are critical for sliding expiration behavior, yet there's no explicit
test that calls them directly to verify they work correctly.
##########
modules/platforms/dotnet/Apache.Extensions.Caching.Ignite/IgniteDistributedCache.cs:
##########
@@ -105,34 +132,30 @@ public async Task SetAsync(string key, byte[] value,
DistributedCacheEntryOption
ArgumentNullException.ThrowIfNull(value);
ArgumentNullException.ThrowIfNull(options);
- if (options.AbsoluteExpiration != null || options.SlidingExpiration !=
null || options.AbsoluteExpirationRelativeToNow != null)
- {
- // TODO: IGNITE-23973 Add expiration support
- throw new ArgumentException("Expiration is not supported.",
nameof(options));
- }
-
- var entry = new CacheEntry(value);
+ (long? expiresAt, long? sliding) = GetExpiration(options);
+ var entry = new CacheEntry(value, expiresAt, sliding);
IKeyValueView<string, CacheEntry> view = await
GetViewAsync().ConfigureAwait(false);
-
await view.PutAsync(transaction: null, key,
entry).ConfigureAwait(false);
}
/// <inheritdoc/>
- public void Refresh(string key)
- {
- ArgumentNullException.ThrowIfNull(key);
-
- // TODO: IGNITE-23973 Add expiration support
- }
+ public void Refresh(string key) =>
+ RefreshAsync(key, CancellationToken.None).GetAwaiter().GetResult();
/// <inheritdoc/>
- public Task RefreshAsync(string key, CancellationToken token)
+ public async Task RefreshAsync(string key, CancellationToken token)
{
ArgumentNullException.ThrowIfNull(key);
- // TODO: IGNITE-23973 Add expiration support
- return Task.CompletedTask;
+ IIgnite ignite = await
_igniteClientGroup.GetIgniteAsync().ConfigureAwait(false);
+
+ var sql = $"UPDATE {_options.TableName} " +
+ $"SET {_options.ExpirationColumnName} =
{_options.SlidingExpirationColumnName} + ? " +
+ $"WHERE {_options.KeyColumnName} = ? AND
{_options.SlidingExpirationColumnName} IS NOT NULL";
Review Comment:
The SQL statement concatenates table and column names directly from
configuration options without any validation or sanitization. While the code
comment at line 238 acknowledges this assumption, this could lead to SQL
injection vulnerabilities if user-supplied values are used for TableName,
KeyColumnName, ExpirationColumnName, or SlidingExpirationColumnName. Consider
adding validation to ensure these values contain only alphanumeric characters
and underscores, or use a different approach to construct the SQL safely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
