Skip to content

add locks around transaction closing #497

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

Merged
jamesmoore merged 8 commits into from
Mar 11, 2026
Merged

add locks around transaction closing #497

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bff66fb
add locks around transaction closing
Mar 8, 2026
10a3101
Initial plan
Copilot Mar 8, 2026
0995b53
Fix transaction synchronization: pass captured transaction to Execute…
Copilot Mar 8, 2026
9be1f17
Merge pull request #498 from jamesmoore/copilot/sub-pr-497
jamesmoore Mar 11, 2026
4a4be81
use Lock
Mar 11, 2026
fffb936
Initial plan
Copilot Mar 11, 2026
c7ac43e
Fix unsynchronized transaction read in ExecuteOnConnection
Copilot Mar 11, 2026
f8b4be6
Merge pull request #499 from jamesmoore/copilot/sub-pr-497-again
jamesmoore Mar 11, 2026
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
50 changes: 37 additions & 13 deletions SDMeta/Cache/SqliteDataSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Dapper;
using Dapper;
using Microsoft.Data.Sqlite;
using Microsoft.Extensions.Logging;
using System;
Expand All @@ -12,6 +12,7 @@ public partial class SqliteDataSource : IImageFileDataSource
const string TableName = "PngFilesv2";
private string FTSTableName = $"FTS5{TableName}";
private SqliteTransaction? transaction;
private readonly object transactionLock = new();

private readonly string[] columns =
[
Expand Down Expand Up @@ -75,13 +76,18 @@ private T ExecuteOnConnection<T>(Func<SqliteConnection, T> func)
{
if (this.transaction?.Connection != null)
{
return func(transaction.Connection);
}
else
{
using var connection = GetConnection();
return func(connection);
lock (transactionLock)
{
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExecuteOnConnection reads this.transaction in the initial if (this.transaction?.Connection != null) check without holding transactionLock. Since BeginTransaction/CommitTransaction now write this.transaction under that lock, an unsynchronized read can observe a stale null and incorrectly fall back to opening a new connection, causing operations to run outside the intended transaction. Consider taking transactionLock before any read of this.transaction (or using Volatile.Read/Volatile.Write) and then branching based on the captured value.

Copilot uses AI. Check for mistakes.
Copy link
Owner Author

@jamesmoore jamesmoore Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

var currentTransaction = this.transaction;
if (currentTransaction?.Connection != null)
{
return func(currentTransaction.Connection);
}
}
}

using var connection = GetConnection();
return func(connection);
}

private string GetInsertSql()
Expand Down Expand Up @@ -257,18 +263,35 @@ private DataRow FromModel(ImageFile info)

public void BeginTransaction()
{
this.transaction ??= GetConnection().BeginTransaction();
lock (transactionLock)
{
this.transaction ??= GetConnection().BeginTransaction();
}
}

public void CommitTransaction()
{
if (this.transaction != null)
SqliteTransaction? transactionToCommit;

lock (transactionLock)
{
var connection = this.transaction.Connection;
this.transaction.Commit();
this.transaction.Dispose();
transactionToCommit = this.transaction;
this.transaction = null;
connection?.Close();
}

if (transactionToCommit == null)
{
return;
}

try
{
transactionToCommit.Commit();
}
finally
{
var connection = transactionToCommit.Connection;
transactionToCommit.Dispose();
connection?.Dispose();
}
}
Expand Down Expand Up @@ -333,3 +356,4 @@ public static string ToCommaSeparated(this IEnumerable<string> list)

internal record struct ColumnDefinition(string Column, string Parameter, string DataType, bool IsPrimaryKey);
}

Loading