Usage

We use a wrapper for NpgsqlTransaction that wraps a transaction/connection pair. In this model, the NpgsqlConnection we acquire lives as long as the transaction. That is, when we commit or rollback the transaction, we also dispose the connection object.

A recent change in our code ensured that the transaction and the connection are closed before calling a post commit hook.

The relevant portions of the code look like this:

public async Task Commit()
{
    await transaction.CommitAsync();
    await DisposeAsync();
    await PostCommit.InvokeAllSequentially(CancellationToken.None);
}

public void Dispose()
{
    transaction.Dispose();
    connection.Dispose();
}

You probably are already imagining how it is used:

await using var transaction = await connectionFactory.BeginTransactionAsync();

/// do something with transaction

await transaction.Commit();

Symptoms

Exceptions we've seen are:

• ObjectDisposedException: NpgsqlTransaction on commit
• The connection is already in state 'Executing'
• IndexOutOfRange / NullReference thrown inside NpgsqlDataReader
• "SslStream disposed" and "Operation canceled" errors while reading

Depending on which exact race scenario was hit.
After a while, we've experienced the connection pool being exhausted.

Cause

Recycling of NpgsqlTransaction introduced in version 5 via #2416

IMO, the behavior of "undisposing" an object is unexpected - quite special, so to say.
I would like to cite:

(...) a Dispose method should be idempotent, such that it's callable multiple times without throwing an exception. Furthermore, subsequent invocations of Dispose should do nothing.

Source: https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose

I understand some of the reasons why this guidance exists, but in no way am promising a complete picture:

• it can be tricky to get single disposal right, especially when you pass on a disposable object A to another disposable object B, and it's unclear whether B.Dispose() will call A.Dispose() or not. Like: Does the reader dispose the file handle / connection?
• historically, in .net framework (a long time ago), some code involving yield return and using could cause double disposal, even if you wrote the code a 100% correct.

My personal stance on this is, that I keep my own disposal code simple. I always trust in the underlying objects and primitives to do their jobs right, I never do isDisposed checks (unless I'm the lowest level of the chain). Until NpgsqlTransaction I never had a problem with this pattern.

The work around is in our code is rather obvious: *do introduce a bool isDisposed check, and prevent double disposal.
It's also not a big thing. I'm mostly reporting because it could cause problems for others, too. And, especially race conditions, can be difficult to diagnose.

Related Issues

• Replace NpgsqlTransaction.IsCompleted with NpgsqlConnection.IsTransactionInProgress #2618
• Fixed using the connector with multiplexing, after it was returned to the pool #3254

Ideas

When this was introduced in #2416, the reason given was performance gain.
The original perf-gain has not been specified with any numbers. Maybe it has become insignificant in the meantime? If so, I suppose the implementation would be simpler w/o reuse, too.

Alternatively, maybe the following pattern could be considered:

1. renaming the current NpgsqlTransaction to ReusableTransaction (and make it internal)
2. expose NgsqlTransaction as a wrapper around ReusableTransaction:

public class NpgsqlTransaction : DbTransaction
{
    ReusableTransaction _reusable;

    internal NpgsqlTransaction(ReusableTransaction reusable)
    {
         _reusable = reusable;
    }

    public void Dispose()
    {
        _reusable?.Dispose();
        _reusable = null;
    }

    ReusableTransaction Reusable => _reusable ?? throw new ObjectDisposedException("transaction has already been disposed");

    // example of actually doing something with the transaction.
    public override void Save(string name)
    {
        Reusable.Save(name);
    }
}

Note-to-self: https://github.com/eCarUp/smartme-cloud/pull/4136