Optimize performance of DeleteExpiresAsync#301
Conversation
|
@copilot 继续工作 |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the DeleteExpiresAsync method to improve performance and reduce deadlock risk by adopting a two-step delete pattern: first selecting IDs of expired messages, then deleting by those IDs. This approach mirrors the pattern from the MySQL reference implementation but uses EF Core LINQ instead of raw SQL.
Key changes:
- Changed from single-query direct delete to two-step pattern (SELECT IDs, then DELETE by IDs)
- Added explicit null check for
ExpiresAtfield instead of using null-forgiving operator - Added early return optimization when no expired records are found
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var ids = await context.PublishedMessages.AsNoTracking() | ||
| .Where(m => m.ExpiresAt != null && m.ExpiresAt < timeout && statusNames.Contains(m.StatusName)) | ||
| .OrderBy(m => m.Id) | ||
| .Select(m => m.Id) | ||
| .Take(batchCount) | ||
| .ToListAsync(token); | ||
|
|
||
| if (ids.Count == 0) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| return await context.PublishedMessages | ||
| .Where(m => ids.Contains(m.Id)) | ||
| .ExecuteDeleteAsync(token); | ||
| } | ||
| else if (table == NetCorePalStorageOptions.ReceivedMessageTableName) | ||
| { | ||
| return await context.ReceivedMessages | ||
| .Where(m => m.ExpiresAt! < timeout && statusNames.Contains(m.StatusName)) | ||
| var ids = await context.ReceivedMessages.AsNoTracking() | ||
| .Where(m => m.ExpiresAt != null && m.ExpiresAt < timeout && statusNames.Contains(m.StatusName)) | ||
| .OrderBy(m => m.Id) | ||
| .Select(m => m.Id) | ||
| .Take(batchCount) | ||
| .ToListAsync(token); | ||
|
|
||
| if (ids.Count == 0) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| return await context.ReceivedMessages | ||
| .Where(m => ids.Contains(m.Id)) | ||
| .ExecuteDeleteAsync(token); | ||
| } |
There was a problem hiding this comment.
The logic for PublishedMessages (lines 300-314) and ReceivedMessages (lines 318-332) is duplicated. This violates the DRY (Don't Repeat Yourself) principle and makes the code harder to maintain.
Consider extracting the common logic into a helper method:
private async Task<int> DeleteExpiredMessagesAsync<TMessage>(
DbSet<TMessage> dbSet,
DateTime timeout,
List<string> statusNames,
int batchCount,
CancellationToken token)
where TMessage : class
{
var ids = await dbSet.AsNoTracking()
.Where(m => EF.Property<DateTime?>(m, "ExpiresAt") != null &&
EF.Property<DateTime?>(m, "ExpiresAt") < timeout &&
statusNames.Contains(EF.Property<string>(m, "StatusName")))
.OrderBy(m => EF.Property<long>(m, "Id"))
.Select(m => EF.Property<long>(m, "Id"))
.Take(batchCount)
.ToListAsync(token);
if (ids.Count == 0)
{
return 0;
}
return await dbSet
.Where(m => ids.Contains(EF.Property<long>(m, "Id")))
.ExecuteDeleteAsync(token);
}Then simplify the if-else blocks to call this helper method.
| var ids = await context.PublishedMessages.AsNoTracking() | ||
| .Where(m => m.ExpiresAt != null && m.ExpiresAt < timeout && statusNames.Contains(m.StatusName)) | ||
| .OrderBy(m => m.Id) | ||
| .Select(m => m.Id) | ||
| .Take(batchCount) | ||
| .ToListAsync(token); | ||
|
|
||
| if (ids.Count == 0) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| return await context.PublishedMessages | ||
| .Where(m => ids.Contains(m.Id)) | ||
| .ExecuteDeleteAsync(token); | ||
| } | ||
| else if (table == NetCorePalStorageOptions.ReceivedMessageTableName) | ||
| { | ||
| return await context.ReceivedMessages | ||
| .Where(m => m.ExpiresAt! < timeout && statusNames.Contains(m.StatusName)) | ||
| var ids = await context.ReceivedMessages.AsNoTracking() | ||
| .Where(m => m.ExpiresAt != null && m.ExpiresAt < timeout && statusNames.Contains(m.StatusName)) | ||
| .OrderBy(m => m.Id) | ||
| .Select(m => m.Id) | ||
| .Take(batchCount) | ||
| .ToListAsync(token); | ||
|
|
||
| if (ids.Count == 0) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| return await context.ReceivedMessages | ||
| .Where(m => ids.Contains(m.Id)) | ||
| .ExecuteDeleteAsync(token); |
There was a problem hiding this comment.
The reference MySQL implementation in the issue uses FOR UPDATE SKIP LOCKED to lock rows during selection, preventing race conditions and deadlocks. The current EF Core implementation lacks this locking mechanism, which could lead to:
- Race conditions: Rows could be modified/deleted between the SELECT and DELETE operations
- Potential deadlocks: Multiple cleanup processes could conflict when deleting the same records
While EF Core doesn't support FOR UPDATE SKIP LOCKED directly in LINQ, consider:
- Using a transaction with appropriate isolation level
- Using raw SQL with database-specific locking clauses for critical deployments
- Documenting this limitation if the behavior is acceptable for the CAP cleanup use case
The two-step approach does improve performance by using a subquery pattern, but the lack of row-level locking differs from the reference implementation's approach to preventing deadlocks.
Thanks for assigning this issue to me. I'm starting to work on it and will keep this PR's description up to date as I form a plan and make progress.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.