Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds locking around the shared transaction field in SqliteDataSource to prevent race conditions when the transaction is committed or disposed while concurrent database operations are still in flight (e.g., during Task.WhenAll in Rescan). The double-checked locking pattern in ExecuteOnConnection ensures safe access to the transaction's connection, and CommitTransaction uses a "capture-under-lock, operate-outside-lock" pattern.
Changes:
- Added a
transactionLockobject and appliedlockblocks inBeginTransaction,CommitTransaction, andExecuteOnConnectionto synchronize access to thetransactionfield. - Refactored
ExecuteOnConnectionto use a double-checked locking pattern that safely falls through to a new connection if the transaction becomes unavailable. - Refactored
CommitTransactionto capture the transaction reference under lock, then commit and dispose outside the lock with proper try/finally cleanup.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| if (this.transaction?.Connection != null) | ||
| { | ||
| return func(transaction.Connection); | ||
| } | ||
| else | ||
| { | ||
| using var connection = GetConnection(); | ||
| return func(connection); | ||
| lock (transactionLock) | ||
| { | ||
| var currentTransaction = this.transaction; | ||
| if (currentTransaction?.Connection != null) | ||
| { | ||
| return func(currentTransaction.Connection); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| using var connection = GetConnection(); | ||
| return func(connection); | ||
| } |
There was a problem hiding this comment.
The locking in ExecuteOnConnection is incomplete: WriteImageFile (line 242) and PostUpdateProcessing (lines 335, 345) read this.transaction inside their lambda callbacks without acquiring transactionLock. While the lambda executed at line 84 (inside the lock) is protected, the same lambda can also execute at line 90 (outside the lock) when the transaction is not available, and in that path this.transaction is read without synchronization.
This is fragile: if BeginTransaction were called by another thread between the fall-through and the lambda execution at line 90, the lambda would read a new transaction belonging to a different connection, causing a connection/transaction mismatch error.
Consider having ExecuteOnConnection also provide the captured transaction to the callback (e.g., Func<SqliteConnection, SqliteTransaction?, T>), so callers use the properly-synchronized reference instead of re-reading the field.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@jamesmoore I've opened a new pull request, #498, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.