Fix dependent transaction failure #2197
Conversation
| @@ -285,16 +285,17 @@ protected virtual void Unlock() | |||
| { | |||
| // Cloned transaction is not disposed "unexpectedly", its status is accessible till context disposal. | |||
| var status = EnlistedTransaction.TransactionInformation.Status; | |||
| if (status != TransactionStatus.Active) | |||
| if (status != TransactionStatus.Active || _preparing) | |||
There was a problem hiding this comment.
The trouble with dependent transactions occurs in the prepare phase, where we are sure the transaction is not (yet) roll-backed.
This way of dodging that trouble looks like one more crutch in this dreaded system transaction management. But I have seen no better.
(Checking the original transaction concrete type is unreliable, because a DependentTransaction own Clone method will not yield another DependentTransaction, but a Transaction. Moreover, knowing it is a dependent transaction will anyway not help knowing whether the transaction is still actually active or not.)
fredericDelaporte
force-pushed
the
GH2172
branch
from
3b7fa03 to
6783ed2
Compare
| @@ -236,7 +236,7 @@ public virtual void Wait() | |||
| // Remove the block then throw. | |||
| Unlock(); | |||
| throw new HibernateException( | |||
| "Synchronization timeout for transaction completion. Either raise {Cfg.Environment.SystemTransactionCompletionLockTimeout}, or this may be a bug in NHibernate."); | |||
| $"Synchronization timeout for transaction completion. Either raise {Cfg.Environment.SystemTransactionCompletionLockTimeout}, or this may be a bug in NHibernate."); | |||
There was a problem hiding this comment.
Trivial bug fix by the way. I do not think it is even worth a mention in commit.
fredericDelaporte
force-pushed
the
GH2172
branch
from
6783ed2 to
daa283e
Compare
fredericDelaporte
force-pushed
the
GH2172
branch
from
daa283e to
064818b
Compare
fredericDelaporte
changed the title
This comment has been minimized.
This comment has been minimized.
fredericDelaporte
changed the title
| var currentTransaction = System.Transactions.Transaction.Current; | ||
| if (!ReferenceEquals(currentTransaction, _originalTransaction) && | ||
| currentTransaction == EnlistedTransaction) | ||
| return currentTransaction.TransactionInformation.Status; |
There was a problem hiding this comment.
This allows chaining multiple dependent transaction usage of the same original transaction on the same session. This means this use case will works with some exception handling, which hurts performances.
But dodging theses exceptions for this dependent case would require to check the current transaction first (instead of trying the original one), which would introduce another exception handling for more regular cases than dependent transaction or rollback.
Fix chaining dependent transaction usage on the same session
fredericDelaporte
force-pushed
the
GH2172
branch
from
ab3507a to
666e4f2
Compare
|
It seems we are getting an async test trouble. Fiddling with |
| using NHibernate.Test.TransactionTest; | ||
| using NUnit.Framework; | ||
|
|
||
| using sysTran = System.Transactions; |
There was a problem hiding this comment.
Ok, I will revert.
I did that due to otherwise having to specify the full namespace everywhere before "sysTran" Transaction because of the conflict with NHibernate Transaction namespace, and I then rather shorten the namespace prefixing.
Remove sysTran
Fix #2172
This is a regression of NH5.0, causing dependent transactions to be unusable.
The trouble with dependent transactions is, they are usually disposed of before completing the actual transaction.
This causes a bug in NHibernate transaction context when checking for the transaction status. It fortunately uses a clone for this, but due to a bug where the status may still be active while the transaction is rollbacked, it does also check the original transaction, and currently assumes that if it is disposed, the transaction is no more active.
This is not true with dependent transactions.
(Well actually the check method just yield null to tell it does not know, but then the code calling it do the "not active" path.)
Since this PR targets 5.2.x, I will delay releasing 5.2.6.
If merged, this PR should be back-ported down to 5.0.x