Description
As part of #2144, _isAlreadyDisposed is set to true earlier in the AdoTransaction.Dispose method than before.
One of the implications of this is that if the DbTransaction throws an exception in its Dispose method, then _isAlreadyDisposed is true, but begun might also still be true. Any attempt to dispose of the associated session will throw an InvalidOperationException with the text Disconnect cannot be called while an explicit transaction is in progress.
The end result is a session with an active transaction, neither of which can be disposed. This in turn causes other transactions to block for long periods of time.
As to why the DbTransaction is throwing an exception, I'll leave that for another issue. (Short summary: the MySQL connector can sometimes throw an exception in Dispose if an async query was cancelled). For now I thought it best to just focus on the fact that an AdoTransaction object enters an inconsistent state if the Dispose method throws an exception for any reason at all.
Having read #2144, my understanding is that the _isAlreadyDisposed field is set earlier to avoid recursive calls to the Dispose method. Another way to achieve the same thing might be to define another field:
AdoTransaction.cs:335
/// <summary>
/// A flage to indicate if <c>Dispose</c> is currently being called.
/// </summary>
private bool _isInDispose;AdoTransaction.cs:364
protected virtual void Dispose(bool isDisposing)
{
using (SessionIdLoggingContext.CreateOrNull(sessionId))
{
if (_isAlreadyDisposed || _isInDispose)
{
// don't dispose of multiple times.
return;
}
try
{
_isInDispose = true;
// free managed resources that are being managed by the AdoTransaction if we
// know this call came through Dispose()
if (isDisposing)
{
if (trans != null)
{
trans.Dispose();
trans = null;
log.Debug("DbTransaction disposed.");
}
if (IsActive && session != null)
{
// Assume we are rolled back
AfterTransactionCompletion(false);
}
// nothing for Finalizer to do - so tell the GC to ignore it
GC.SuppressFinalize(this);
}
// free unmanaged resources here
_isAlreadyDisposed = true;
}
finally
{
_isInDispose = false;
}
}
}If this is acceptable, I'm happy to submit a PR. Alternatively, if there is a better way to achieve this, I'm happy to change the approach.