DbDataReader left open when cancellation occurs in AdvanceAsync after ExecuteReaderAsync

[drakke]

Description

There's a risk DbDataReader won't be disposed of in GetResultSetAsync method within the src\NHibernate\Async\Loader\Loader.cs class. When a cancellation token is canceled after the DbDataReader is successfully obtained from session.Batcher.ExecuteReaderAsync() but during the AdvanceAsync method call, the reader is not closed.

The issue occurs due to the catch block for OperationCanceledException that throws without closing the reader:

catch (OperationCanceledException) { throw; }
catch (Exception sqle)
{
    ADOExceptionReporter.LogExceptions(sqle);
    session.Batcher.CloseCommand(st, rs);
    throw;
}

Steps to Reproduce

I manually changed the NH source code to force the error to happen. Adding the following right after the DataReader initially gets set:

rs = await (session.Batcher.ExecuteReaderAsync(st, cancellationToken)).ConfigureAwait(false);

// Added code on row 1064 to force OperationCanceledException:
CancellationTokenSource cts = new CancellationTokenSource();
cts.Cancel();
cts.Token.ThrowIfCancellationRequested();

Then I added this test to src\NHibernate.Test\Async\Legacy\SQLLoaderTest.cs:

[Test]
public void ExpiredCancellationTokenThrowsOperationCanceledException()
{
    if (Dialect is Oracle8iDialect)
    {
        return;
    }

    Assert.ThrowsAsync<OperationCanceledException>(() => InnerExpiredCancellationTokenThrowsOperationCanceledException());
}

public async Task InnerExpiredCancellationTokenThrowsOperationCanceledException()
{
    using ISession session = OpenSession();
    using ITransaction txn = session.BeginTransaction();

    IQuery q = session.CreateSQLQuery(@"
        SELECT TOP 100000 a.*, b.* 
        FROM sys.objects a
        CROSS JOIN sys.objects b
        ORDER BY a.name");
    await (q.ListAsync());
}

This leads to an unexpected InvalidOperationException while rolling back the transaction rather than the expected OperationCanceledException.

  Message: 
      Expected: <System.OperationCanceledException>
      But was:  <System.InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first.
       at System.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlCommand command)
       at System.Data.SqlClient.SqlInternalTransaction.Rollback()
       at System.Data.SqlClient.SqlInternalTransaction.Dispose(Boolean disposing)
       at System.Data.SqlClient.SqlInternalTransaction.Dispose()
       at System.Data.SqlClient.SqlTransaction.Dispose(Boolean disposing)
       at NHibernate.Transaction.AdoTransaction.Dispose(Boolean isDisposing) in C:\dev\nhibernate-core\src\NHibernate\Transaction\AdoTransaction.cs:line 383
       at NHibernate.Transaction.AdoTransaction.Dispose() in C:\dev\nhibernate-core\src\NHibernate\Transaction\AdoTransaction.cs:line 352
       at NHibernate.Test.Legacy.SQLLoaderTestAsync.InnerExpiredCancellationTokenThrowsOperationCanceledException() in C:\dev\nhibernate-core\src\NHibernate.Test\Async\Legacy\SQLLoaderTest.cs:line 92
       at NUnit.Framework.Internal.TaskAwaitAdapter.GenericAdapter`1.BlockUntilCompleted()
       at NUnit.Framework.Internal.MessagePumpStrategy.NoMessagePumpStrategy.WaitForCompletion(AwaitAdapter awaiter)
       at NUnit.Framework.Internal.AsyncToSyncAdapter.Await(Func`1 invoke)
       at NUnit.Framework.Assert.ThrowsAsync(IResolveConstraint expression, AsyncTestDelegate code, String message, Object[] args)>

Reproducing without manually canceling the token

It is not consistent, but I was able to reproduce with this test after 1-25 iterations:

public async Task InnerExpiredCancellationTokenThrowsOperationCanceledException()
{
    using var cts = new CancellationTokenSource(100);
    using ISession session = OpenSession();
    using ITransaction txn = session.BeginTransaction();

    IQuery q = session.CreateSQLQuery(@"
        SELECT TOP 100000 a.*, b.* 
        FROM sys.objects a
        CROSS JOIN sys.objects b
        ORDER BY a.name");
    await (q.ListAsync(cts.Token));
}

Possible Resolution

This seems to be generated code, but the goal would be to somehow make sure at least session.Batcher.CloseReader(rs); gets called in case of OperationCanceledException.

catch (OperationCanceledException)
{
    session.Batcher.CloseReader(rs);
    throw; 
}

It might be that session.Batcher.CloseCommand(st, rs); should be used, as in the generic catch block? Calling any of those would ensure the DbDataReader gets closed which would allow a transaction to rollback without encountering InvalidOperationException that hides the original exception.

Related issues

maca88/AsyncGenerator#90
dotnet/SqlClient#26 (Makes this less likely to happen)