V10.0.2 optimizations #3
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughA refactor replacing an event-driven write-queue with interceptor-driven concurrency management, adding DI-friendly factories and registration helpers, retry/lock-oriented write paths, connection/pragmas caching, a project version bump to 10.0.2, and extensive documentation and packaging metadata updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Context as ThreadSafeSqliteContext
participant Enhancer as SqliteConnectionEnhancer
participant Interceptor as SqliteConcurrencyInterceptor
participant DB as SQLite
App->>Context: BulkInsertOptimizedAsync(entities)
Context->>Enhancer: GetWriteLock(connectionString)
Enhancer-->>Context: SemaphoreSlim (write lock)
Context->>Context: Acquire lock (await)
Context->>Context: Begin transaction (SERIALIZABLE)
Context->>Enhancer: GetInterceptor(connectionString, options)
Enhancer-->>Context: SqliteConcurrencyInterceptor
Context->>DB: EF Core commands (go through Interceptor)
Interceptor->>Enhancer: ConnectionOpened -> ApplyRuntimePragmas
Interceptor->>DB: Upgrade BEGIN -> BEGIN IMMEDIATE (when applicable)
DB-->>Interceptor: Responses
Context->>Context: Commit transaction
Context->>Enhancer: Release lock
Context-->>App: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/ci.yml (1)
18-22: Critical issue: Workflow will fail —packages.lock.jsonis required but missing.The
cache: trueparameter inactions/setup-dotnet@v4requires apackages.lock.jsonlock file to exist in the repository root. When the file is not found, the setup step fails and blocks the entire build. No lock file currently exists in this repository.To fix this:
- Generate
packages.lock.jsonby runningdotnet restore --locked-modelocally, then commit it to the repository, OR- Remove
cache: truefrom the workflow if you don't want to maintain lock files.If you choose to use caching, ensure
packages.lock.jsonis committed before this workflow change is merged.EntityFrameworkCore.Sqlite.Concurrency/src/ThreadSafeFactory.cs (1)
133-148: Resource leak if context creation fails after connection opens.If
Activator.CreateInstancethrows (line 148), the openedSqliteConnection(line 135) is never disposed. Wrap in try-catch or use a pattern that ensures cleanup.🔧 Proposed fix
public static async Task<TContext> CreateContextWithSharedConnectionAsync<TContext>( string connectionString, Action<SqliteConcurrencyOptions>? configure = null) where TContext : DbContext { var options = new SqliteConcurrencyOptions(); configure?.Invoke(options); var enhancedConnectionString = SqliteConnectionEnhancer .GetOptimizedConnectionString(connectionString); // Create a shared connection var sharedConnection = new SqliteConnection(enhancedConnectionString); - await sharedConnection.OpenAsync(); - - // Apply runtime optimizations - SqliteConnectionEnhancer.ApplyRuntimePragmas(sharedConnection, options); - - // We are not using the extension method here because we are passing an already open connection. - var optionsBuilder = new DbContextOptionsBuilder<TContext>() - .UseSqlite(sharedConnection); - - // Manually add the interceptor for write queue - var interceptor = SqliteConnectionEnhancer.GetInterceptor(enhancedConnectionString, options); - optionsBuilder.AddInterceptors(interceptor); - - return (TContext)Activator.CreateInstance(typeof(TContext), optionsBuilder.Options)!; + try + { + await sharedConnection.OpenAsync(); + + // Apply runtime optimizations + SqliteConnectionEnhancer.ApplyRuntimePragmas(sharedConnection, options); + + var optionsBuilder = new DbContextOptionsBuilder<TContext>() + .UseSqlite(sharedConnection); + + var interceptor = SqliteConnectionEnhancer.GetInterceptor(enhancedConnectionString, options); + optionsBuilder.AddInterceptors(interceptor); + + return (TContext)Activator.CreateInstance(typeof(TContext), optionsBuilder.Options)!; + } + catch + { + await sharedConnection.DisposeAsync(); + throw; + } }EntityFrameworkCore.Sqlite.Concurrency/src/ThreadSafeSqliteContext.cs (2)
185-199: User-configured options are ignored.The
Optionsproperty always creates a newSqliteConcurrencyOptions()with defaults if_optionsis null. User configurations passed to constructors are never captured, soMaxRetryAttemptswill always be 3 (default), ignoring any custom settings.🔧 Proposed fix
Capture options in constructors or retrieve from the DbContext configuration:
+ private SqliteConcurrencyOptions? _options; + public ThreadSafeSqliteContext(string connectionString) { _connectionString = SqliteConnectionEnhancer.GetOptimizedConnectionString(connectionString); _writeLock = SqliteConnectionEnhancer.GetWriteLock(_connectionString); + _options = new SqliteConcurrencyOptions(); // Or accept as parameter } public ThreadSafeSqliteContext(DbContextOptions options) : base(options) { var extension = options.FindExtension<SqliteOptionsExtension>(); if (extension?.ConnectionString != null) { _connectionString = SqliteConnectionEnhancer.GetOptimizedConnectionString(extension.ConnectionString); _writeLock = SqliteConnectionEnhancer.GetWriteLock(_connectionString); } // TODO: Extract SqliteConcurrencyOptions from options if available }
173-181: O(n²) complexity in bulk insert batching.Using
Skip(i).Take(batchSize)inside a loop creates O(n²) complexity for large entity lists sinceSkipmust enumerate all preceding elements on each iteration.🔧 Proposed fix using chunking
public async Task BulkInsertSafeAsync<T>( IList<T> entities, CancellationToken ct = default) where T : class { await ExecuteWriteAsync(async ctx => { - // Batch inserts - var batchSize = 1000; - for (int i = 0; i < entities.Count; i += batchSize) - { - var batch = entities.Skip(i).Take(batchSize).ToList(); - await ctx.AddRangeAsync(batch, ct); - await ctx.SaveChangesAsync(ct); - } + const int batchSize = 1000; + for (int i = 0; i < entities.Count; i += batchSize) + { + var count = Math.Min(batchSize, entities.Count - i); + // Use list slicing for O(1) access + var batch = entities is List<T> list + ? list.GetRange(i, count) + : entities.Skip(i).Take(count).ToList(); + await ctx.AddRangeAsync(batch, ct); + await ctx.SaveChangesAsync(ct); + } }, ct); }Or use .NET's
Chunkmethod if targeting .NET 6+:foreach (var batch in entities.Chunk(1000)) { await ctx.AddRangeAsync(batch, ct); await ctx.SaveChangesAsync(ct); }
🤖 Fix all issues with AI agents
In `@EntityFrameworkCore.Sqlite.Concurrency/doc/v10_0_0.md`:
- Line 135: The doc currently recommends setting
concurrencyOptions.UseWriteQueue = true but the PR states the system moved from
an event-driven write queue to interceptor-based concurrency management; update
the documentation around the concurrencyOptions.UseWriteQueue setting to clarify
its status: state whether UseWriteQueue is deprecated or retained for backward
compatibility, describe any remaining effects it has under the new interceptor
model, and replace or augment the recommendation with guidance to use the
interceptor-based approach (mention the concurrencyOptions object and the
UseWriteQueue property and note the interceptor-based alternative) so the text
is consistent with the architectural change.
- Line 22: The phrase "Guaranteed 100% write reliability and up to 10x faster
bulk operations." is an absolute claim; replace it with a softened, factual
statement — e.g. change the sentence in the v10_0_0.md content that currently
reads "Guaranteed 100% write reliability and up to 10x faster bulk operations."
to something like "Designed for high write reliability and up to 10x faster bulk
operations." or "Improved write reliability and up to 10x faster bulk operations
in typical scenarios." Ensure the revised wording removes "Guaranteed 100%" and
preserves the performance claim while avoiding absolute guarantees.
- Line 73: Replace the emphasized line "*Benchmark environment: .NET 10, Windows
11, Intel i7-13700K, 32GB RAM*" with a proper heading and plain text body:
create a heading like "Benchmark environment" (e.g., "### Benchmark
environment") and move the environment details to the next line as normal
paragraph text so the note is part of the document structure rather than
emphasized inline.
- Line 168: The migration guide currently references BulkInsertSafeAsync() which
is inconsistent with the primary docs; update the text to recommend
BulkInsertOptimizedAsync() instead. Locate the mention of BulkInsertSafeAsync
(used on ThreadSafeSqliteContext) and replace it with BulkInsertOptimizedAsync
(the method on SqliteConcurrencyExtensions) so the v10_0_0.md guide aligns with
README, QUICKSTART.md, and MemoryPackExtensions.
- Around line 116-121: The example uses a non-existent options property
(options.EnablePerformanceOptimizations) when calling
ThreadSafeFactory.CreateContext; update the example to either remove the
configuration lambda or replace that line with a valid SqliteConcurrencyOptions
property (e.g., set options.BusyTimeout or options.MaxRetryAttempts) so the
CreateContext call matches the actual SqliteConcurrencyOptions API, keeping
ExecuteWithRetryAsync usage unchanged.
- Around line 85-92: The documentation example incorrectly shows a non-existent
BulkConfig and properties (BatchSize, PreserveInsertOrder, EnableStreaming,
UseOptimalTransactionSize); update the doc to match the real API by replacing
the call with the actual signature—e.g. show usage as await
_context.BulkInsertSafeAsync(modernRecords, cancellationToken) and remove the
BulkConfig block (or alternatively, if you prefer the documented options,
refactor the BulkInsertSafeAsync<T> method to accept a config parameter and
implement the listed properties on a new BulkConfig type and wire them into
BulkInsertSafeAsync<T>).
In `@EntityFrameworkCore.Sqlite.Concurrency/doc/v10_0_2.md`:
- Around line 1-56: The markdown has inline "csharp" labels and un-fenced code
snippets causing rendering issues; fix by converting each example into proper
fenced code blocks with language tags (```csharp ... ```). Update occurrences
around the sample lines referencing _connectionStringCache initialization,
builder["Page Size"] assignment, command.CommandText = pragmas.ToString(),
PRAGMA cache_size example, the ApplyBulkOptimizationPragmas method mention, and
the VACUUM snippet so each code fragment is wrapped in ```csharp fences and
explanatory text stays outside the fences for consistent rendering.
In `@EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyExtensions.cs`:
- Around line 109-117: The batch insert loop (using entities.Chunk and calling
context.AddRangeAsync and context.SaveChangesAsync) leaves all inserted entities
tracked in the DbContext, causing memory to grow; after each successful
SaveChangesAsync call, clear the context's change tracker (e.g., call
context.ChangeTracker.Clear() or explicitly detach entries) so previously saved
entities are removed from tracking before processing the next batch; ensure this
is done inside the foreach after SaveChangesAsync to preserve batching memory
benefits.
In `@EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyInterceptor.cs`:
- Around line 161-164: Remove the two invalid methods
TransactionExplictlyStarted and TransactionExplictlyStartedAsync from
SqliteConcurrencyInterceptor: they are not part of EF Core's
IDbTransactionInterceptor and should not be implemented; locate the methods
named TransactionExplictlyStarted and TransactionExplictlyStartedAsync in
SqliteConcurrencyInterceptor and delete both their declarations (including the
XML doc lines), leaving the rest of the class and valid
IDbTransactionInterceptor implementations intact.
In `@EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConnectionEnhancer.cs`:
- Around line 60-63: GetInterceptor currently uses
_interceptors.GetOrAdd(connectionString, ...) which lets the first caller's
SqliteConcurrencyOptions silently win; update GetInterceptor to detect
mismatched options: attempt to get an existing interceptor from _interceptors by
connectionString, and if present compare its configured options with the
incoming SqliteConcurrencyOptions (expose a property or method on
SqliteConcurrencyInterceptor to obtain its options if needed); if they differ,
throw a clear ArgumentException describing the conflict (or alternatively change
the cache key to include a hash of relevant SqliteConcurrencyOptions fields if
you prefer multiple cached variants per connectionString). Also add XML doc on
GetInterceptor documenting the behavior and that callers must use consistent
options or expect an exception.
- Around line 100-116: The code marks a database as initialized via
_initializedDatabases.TryAdd(dataSource, true) before running
initCommand.ExecuteNonQuery(), so if ExecuteNonQuery throws the dataSource
remains marked and will never retry; fix by ensuring the marker is only set
after successful execution (or removed on failure): wrap the pragma execution in
a try/catch inside the lock around _pragmaLocks.GetOrAdd(dataSource,...), call
initCommand.ExecuteNonQuery() first, and then add
_initializedDatabases.TryAdd(dataSource, true) (or in the catch call
_initializedDatabases.TryRemove(dataSource, out _)) so that failures do not
leave the database marked as initialized.
🧹 Nitpick comments (10)
EntityFrameworkCore.Sqlite.Concurrency/doc/v10_0_0.md (2)
153-153: Consider proper unit formatting.Grammar tools suggest adding a space between the number and unit: "1 ms" instead of "1ms" for better readability and adherence to SI unit conventions.
📝 Suggested fix
-A: Less than 1ms per write operation—negligible compared to the performance gains from optimized bulk operations and parallel reads. +A: Less than 1 ms per write operation—negligible compared to the performance gains from optimized bulk operations and parallel reads.
182-182: Consider hyphenating compound adjective.When "open source" is used as a compound adjective modifying "projects," it should be hyphenated: "open-source projects."
📝 Suggested fix
-EntityFrameworkCore.Sqlite.Concurrency is licensed under the MIT License. Free for commercial use, open source projects, and enterprise applications. +EntityFrameworkCore.Sqlite.Concurrency is licensed under the MIT License. Free for commercial use, open-source projects, and enterprise applications..github/workflows/deploy.yml (1)
29-33: Ensure the cache key covers central package files.With
cache: trueon Line 33, confirm the cache key includes any central package files (e.g.,Directory.Packages.props,NuGet.config) so cache hits don’t go stale when dependencies change.♻️ Optional cache-dependency-path addition (only if these files exist)
- name: Setup .NET uses: actions/setup-dotnet@v4 with: dotnet-version: '10.0.x' cache: true + cache-dependency-path: | + **/*.csproj + **/*.fsproj + **/*.vbproj + **/*.sln + Directory.Packages.props + NuGet.configEntityFrameworkCore.Sqlite.Concurrency/src/ExtensionMethods/SqliteConcurrencyServiceCollectionExtensions.cs (1)
21-34: Minor: Use discard for unusedproviderparameter.The
providerparameter in the lambda is unused. Consider using a discard to make the intent clearer.♻️ Suggested fix
- services.AddDbContext<TContext>((provider, options) => + services.AddDbContext<TContext>((_, options) => { options.UseSqliteWithConcurrency(connectionString, configure); }, contextLifetime);EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyExtensions.cs (2)
1-5: Remove unused import.
Microsoft.Extensions.DependencyInjection(line 5) is not used in this file.♻️ Suggested fix
using System.Data; using EntityFrameworkCore.Sqlite.Concurrency.Models; using Microsoft.EntityFrameworkCore; using Microsoft.Data.Sqlite; -using Microsoft.Extensions.DependencyInjection;
58-77: Consider capping the retry delay.With exponential backoff using
100 * Math.Pow(2, attempt), delays grow quickly. FormaxRetries=5, the final delay would be 3.2 seconds; formaxRetries=10, it would be ~102 seconds. Consider capping the maximum delay.♻️ Suggested fix
catch (SqliteException ex) when (ex.SqliteErrorCode == 5 && attempt < maxRetries) { attempt++; - await Task.Delay(TimeSpan.FromMilliseconds(100 * Math.Pow(2, attempt)), cancellationToken); + var delay = Math.Min(100 * Math.Pow(2, attempt), 5000); // Cap at 5 seconds + await Task.Delay(TimeSpan.FromMilliseconds(delay), cancellationToken); }README.md (1)
111-116: Redundant retry configuration in example.The example shows
MaxRetryAttempts = 5in the options (line 113), then passesmaxRetries: 5again toExecuteWithRetryAsync(line 115). This is potentially confusing—clarify whether the method parameter overrides the configured option or if only one should be specified.📝 Suggested clarification
using var context = ThreadSafeFactory.CreateContext<AppDbContext>( "Data Source=app.db", - options => options.MaxRetryAttempts = 5); + options => options.MaxRetryAttempts = 5); // Default retry attempts - return await context.ExecuteWithRetryAsync(operation, maxRetries: 5); + return await context.ExecuteWithRetryAsync(operation); // Uses configured MaxRetryAttemptsEntityFrameworkCore.Sqlite.Concurrency/src/ThreadSafeFactory.cs (1)
108-113: Null-forgiving operator without validation.
Activator.CreateInstancecan returnnullif the type lacks a suitable constructor. Consider adding a null check or throwing a descriptive exception.🔧 Proposed fix
public static TContext CreateContextFromOptions<TContext>( DbContextOptions<TContext> options) where TContext : DbContext { - return (TContext)Activator.CreateInstance(typeof(TContext), options)!; + return (TContext?)Activator.CreateInstance(typeof(TContext), options) + ?? throw new InvalidOperationException( + $"Failed to create instance of {typeof(TContext).Name}. Ensure it has a constructor accepting DbContextOptions<{typeof(TContext).Name}>."); }EntityFrameworkCore.Sqlite.Concurrency/src/ThreadSafeSqliteContext.cs (1)
108-127: Double-release risk on SQLITE_BUSY retry.When
SQLITE_BUSYis caught (lines 108-119), the lock is released at lines 111-112. However,IsWriteLockHeld.Valueis set tofalsebefore releasing, so thefinallyblock (lines 120-127) won't release again. This is correct but fragile—if the code is modified, this invariant could break.Consider restructuring to make the single-release guarantee explicit:
🔧 Suggested restructure
catch (SqliteException ex) when (ex.SqliteErrorCode == 5) // SQLITE_BUSY { - // Release lock before retry wait - SqliteConnectionEnhancer.IsWriteLockHeld.Value = false; - WriteLock.Release(); - attempt++; if (attempt >= maxRetryAttempts) throw new TimeoutException($"Database busy timeout after {attempt} retries", ex); + // Release lock before retry wait (finally won't release since we continue the loop) + SqliteConnectionEnhancer.IsWriteLockHeld.Value = false; + WriteLock.Release(); + await Task.Delay(100 * (int)Math.Pow(2, attempt), ct); + continue; // Explicit continue to re-enter loop and re-acquire lock }EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyInterceptor.cs (1)
13-27: Unused_writeLockfield.The
_writeLockfield is initialized but never used within this interceptor. If write locking is handled elsewhere (e.g.,ThreadSafeSqliteContext), consider removing this field to avoid confusion.🔧 Proposed fix
public class SqliteConcurrencyInterceptor : DbCommandInterceptor, IDbConnectionInterceptor, IDbTransactionInterceptor { private readonly SqliteConcurrencyOptions _options; - private readonly SemaphoreSlim _writeLock; private readonly string _connectionString; public SqliteConcurrencyInterceptor(SqliteConcurrencyOptions options, string connectionString) { _options = options; _connectionString = connectionString; - _writeLock = SqliteConnectionEnhancer.GetWriteLock(connectionString); }
EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyExtensions.cs
Show resolved
Hide resolved
| /// <inheritdoc /> | ||
| public InterceptionResult TransactionExplictlyStarted(DbConnection connection, TransactionEndEventData eventData, InterceptionResult result) => result; | ||
| /// <inheritdoc /> | ||
| public ValueTask<InterceptionResult> TransactionExplictlyStartedAsync(DbConnection connection, TransactionEndEventData eventData, InterceptionResult result, CancellationToken cancellationToken = default) => new(result); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
EF Core IDbTransactionInterceptor TransactionExplicitly method name documentation
💡 Result:
There is no TransactionExplicitly… method on EF Core’s IDbTransactionInterceptor / DbTransactionInterceptor.
In EF Core, transaction interception is exposed via methods like:
TransactionStarting/TransactionStarted– called around EF callingBeginTransaction(...)(this includes EF-created transactions, which may be implicit). [1]TransactionUsed– called when you explicitly attach an existing transaction viaRelationalDatabaseFacadeExtensions.UseTransaction(...)/UseTransactionAsync(...). [2]TransactionCommitting/TransactionCommitted,TransactionRollingBack/TransactionRolledBack, plus savepoint methods. [2][3]
Docs to reference:
IDbTransactionInterceptor.TransactionStarting(...)(API doc) [1]DbTransactionInterceptormethod list (showsTransactionUsed, commit/rollback, etc.) [2]- Example commit hook doc:
DbTransactionInterceptor.TransactionCommitting(...)[3]
Sources: [1] (learn.microsoft.com) [2] (learn.microsoft.com) [3] (learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.diagnostics.idbtransactioninterceptor.transactionstarting?view=efcore-10.0&utm_source=openai
- 2: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.diagnostics.dbtransactioninterceptor?view=efcore-10.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.diagnostics.dbtransactioninterceptor.transactioncommitting?view=efcore-9.0&utm_source=openai
Remove the TransactionExplictlyStarted and TransactionExplictlyStartedAsync methods—they do not exist in EF Core's IDbTransactionInterceptor interface.
The method name is not a typo but rather a method that has no correspondence in the interface. EF Core's IDbTransactionInterceptor defines methods like TransactionStarted, TransactionUsed, TransactionCommitting, TransactionCommitted, and others, but no variant of "TransactionExplicitly…". These methods should be removed as they are either dead code or incorrect implementations.
🤖 Prompt for AI Agents
In `@EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyInterceptor.cs`
around lines 161 - 164, Remove the two invalid methods
TransactionExplictlyStarted and TransactionExplictlyStartedAsync from
SqliteConcurrencyInterceptor: they are not part of EF Core's
IDbTransactionInterceptor and should not be implemented; locate the methods
named TransactionExplictlyStarted and TransactionExplictlyStartedAsync in
SqliteConcurrencyInterceptor and delete both their declarations (including the
XML doc lines), leaving the rest of the class and valid
IDbTransactionInterceptor implementations intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConnectionEnhancer.cs`:
- Around line 64-81: GetInterceptor currently has a TOCTOU where two threads can
race and bypass options validation; change it to always call
_interceptors.GetOrAdd(...) first to obtain the interceptor instance (var
interceptor = _interceptors.GetOrAdd(connectionString, cs => new
SqliteConcurrencyInterceptor(options, cs))) and then compare interceptor.Options
(type SqliteConcurrencyOptions) to the incoming options using the existing
equality logic (and FormatOptions for message) and throw the ArgumentException
if they differ; this ensures options are validated against the actual stored
SqliteConcurrencyInterceptor and prevents silent mismatches while keeping
interceptor creation via GetOrAdd.
♻️ Duplicate comments (1)
EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyInterceptor.cs (1)
166-169: Remove invalid interface methods:TransactionExplictlyStartedandTransactionExplictlyStartedAsync.These methods do not exist in EF Core's
IDbTransactionInterceptorinterface. The interface definesTransactionStarted,TransactionUsed,TransactionCommitting, etc., but no "TransactionExplicitly..." variant. These appear to be dead code that will never be called.🔧 Suggested fix - remove both methods
/// <inheritdoc /> public Task ReleasedSavepointAsync(DbTransaction transaction, TransactionEventData eventData, CancellationToken cancellationToken = default) => Task.CompletedTask; - /// <inheritdoc /> - public InterceptionResult TransactionExplictlyStarted(DbConnection connection, TransactionEndEventData eventData, InterceptionResult result) => result; - /// <inheritdoc /> - public ValueTask<InterceptionResult> TransactionExplictlyStartedAsync(DbConnection connection, TransactionEndEventData eventData, InterceptionResult result, CancellationToken cancellationToken = default) => new(result); // --- Connection Management (IConnectionInterceptor) ---
🧹 Nitpick comments (2)
EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyExtensions.cs (1)
64-76: Cancellation token should be checked before retry.If the caller cancels the operation, the loop will only exit when
Task.DelaythrowsOperationCanceledException. Adding an explicit cancellation check improves responsiveness.♻️ Suggested improvement
int attempt = 0; while (true) { + cancellationToken.ThrowIfCancellationRequested(); try { return await operation(context); } catch (SqliteException ex) when (ex.SqliteErrorCode == 5 && attempt < maxRetries) { attempt++; await Task.Delay(TimeSpan.FromMilliseconds(100 * Math.Pow(2, attempt)), cancellationToken); } }EntityFrameworkCore.Sqlite.Concurrency/src/SqliteConcurrencyInterceptor.cs (1)
11-32: Unused field:_writeLockis stored but never used.The
_writeLocksemaphore is retrieved fromSqliteConnectionEnhancer.GetWriteLockand stored, but this interceptor never callsWaitAsyncorReleaseon it. If locking is handled elsewhere (e.g.,BulkInsertOptimizedAsync), consider removing this field to avoid confusion.♻️ Suggested cleanup
public class SqliteConcurrencyInterceptor : DbCommandInterceptor, IDbConnectionInterceptor, IDbTransactionInterceptor { private readonly SqliteConcurrencyOptions _options; - private readonly SemaphoreSlim _writeLock; private readonly string _connectionString; /// <summary> /// Gets the concurrency options configured for this interceptor. /// </summary> public SqliteConcurrencyOptions Options => _options; public SqliteConcurrencyInterceptor(SqliteConcurrencyOptions options, string connectionString) { _options = options; _connectionString = connectionString; - _writeLock = SqliteConnectionEnhancer.GetWriteLock(connectionString); }
Summary by CodeRabbit
New Features
Improvements
Documentation
Packaging
✏️ Tip: You can customize this high-level summary in your review settings.