Question: SQL Transaction handling and already closed behaviour

[dusangojkov]

target framework: net8.0
nuget: PetaPoco.Compiled 6.0.677
database: SQL Server

Issue Summary:
I'm encountering an InvalidOperationException stating "Transaction has already been committed or rolled back" when using transactions with asynchronous operations in PetaPoco. This happens even though I think I am properly awaiting the async methods within the transaction scope.

I am having a problem where I do not understand how it could happend that the transaction is already closed, I was woundering if you could help me understand how the error message I received is possible in my case.

Any guidance or insights you can provide would be greatly appreciated, as I want to ensure that the transaction handling is robust and safe for production use.

Exception stacktrace: 
at System.Data.SqlClient.SqlTransaction.ZombieCheck() 
at System.Data.SqlClient.SqlTransaction.Rollback() 
at PetaPoco.Database.CleanupTransaction() 
at PetaPoco.Database.CompleteTransaction() 
at PetaPoco.Database.AbortTransaction() 
at PetaPoco.Transaction.Dispose() 
at ValueRetail.DataService.Persistance.Connections.DataTransaction.Dispose(Boolean disposing)
.. at DataTransaction.cs:line (at: Transaction.Dispose(); inside Dispose method)
.. at DataRetentionService.AnonymiseRetentionRecords() (at the 'return updated;' line)

In my persistance layer I have wrapped the PetaPoco implementation like this:

public sealed class DataService(IOptions<AppSettings> config) : IDataService
{
    /// <summary>
    /// Gets regular connection. Use with "using" statement to propertly dispose the connection.
    /// </summary>
    /// <returns>Returns regular connection to the database.</returns>
    public IDataConnection CreateConnection() => new DataConnection(config.Value.Database.ConnectionString);

    /// <summary>
    /// Gets transactional connection. Use with "using" statement to propertly dispose the connection.
    /// </summary>
    /// <returns>Returns transactional connection to the database.</returns>
    public IDataTransaction CreateTransaction() => new DataTransaction(config.Value.Database.ConnectionString);
}

This is the centralized entry point where you can create a new object for either a regular connection or a transaction. The object itself is not disposable, as it is injected through the dependency injection container. However, when creating either a regular connection or a transaction, you should use the using statement to ensure they are properly disposed of.

The implementation for DataTransaction class looks like this:

internal sealed class DataTransaction : IDataTransaction
{
    private Database? Database { get; set; }
    private Transaction? Transaction { get; set; }

    public DataTransaction(string connectionString)
    {
        Database = new(connectionString, "SqlServer");
        Transaction = new Transaction(Database);

        Data = new RepositoryContext(Database);
    }

    // Have all repositories inside.
    public IDataContext Data { get; }

    public void Commit() => Transaction?.Complete();

    #region [Disposing]
    public void Dispose(bool disposing)
    {
        if (disposing)
        {
            if (Transaction != null)
            {
                Transaction.Dispose();
                Transaction = null;
            }

            if (Database != null)
            {
                Database.Dispose();
                Database = null;
            }
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    #endregion
}

The internal repository context (IDataContext) has mutliple repositories in it. This object is created inside the DataTransaction or DataConnection object.

/// <summary>
/// A single point extry for all repositories within the system.
/// </summary>
internal class RepositoryContext(Database database) : IDataContext
{
    public IDataRetentionRepository AnalysisRepository { get; } = new DataRetentionRepository(database);
}

internal class DataRetentionRepository(Database database) : BaseRepository(database), IDataRetentionRepository
{
  public async Task<int> AnonymiseRetentionRecords()
  {
      // START ANONYMIZATION:
      await Database.ExecuteAsync(@"
          ... raw sql ...");
  
      await Database.ExecuteAsync(@"
          ... raw sql ...");
  
      await Database.ExecuteAsync(@"
          ... raw sql ...");
  
      await Database.ExecuteAsync(@"
          ... raw sql ...");
  
      return await Database.ExecuteAsync(@"
         ... raw sql ...
              )");
  }
}

And finally here is the example of using it:

public async Task<int> AnonymiseRetentionRecords()
{
    using var transaction = _dataService.CreateTransaction();
    var updated = await transaction.Data.AnalysisRepository.AnonymiseRetentionRecords();

    transaction.Commit();

    // Here the exception stack trace began:
    return updated;
}

From my understanding:

• Since the CreateTransaction() method always creates a new Transaction object, I don’t need to worry about the transaction being closed elsewhere.
• If an exception occurs at the database level or within the repository's internal methods, the transaction will remain open until the using statement disposes of it. Any exceptions will be caught later in the system's try-catch mechanism. I’ve tested this by intentionally triggering exceptions at the repository level and testing database timeout exceptions.
• This issue only occurred once in the UAT environment. However, I am hesitant to use this code in the production environment until I am certain there are no potential pitfalls in my logic and that everything is being handled properly.

[asherber]

There's an awful lot of user scaffolding in this code. If you suspect there's an issue with PetaPoco, can you reduce this down to a minimal reproducible example that calls the library directly?