TransactionScope issue

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Use a TransactionScope around a DataContext.
2. Make a stored procedure call or some write operation.
3. Leaving the scope of the DataContext causes exception.

What is the expected output? What do you see instead?
Expected: no exception.
Actual: System.InvalidOperationException : Disconnect cannot be called while a 
transaction is in progress.

See the unit tests that reproduce the issue in the attached Test.cs.patch (for 
the Tracker sample).
Note that the errors are slightly different for the tests.

I created a fix, it can be found in StateSession.cs.patch. I removed the call 
to Session.Close() in CodeSmith.Data.NHibernate.StateSession.Dispose() since 
NHibernate itself does not close the session if there is an open 
TransactionContext.
Here an extract from NHibernate.Impl.SessionImpl:
public void Dispose()
{
    using (new SessionIdLoggingContext(base.SessionId))
    {
        SessionImpl.log.Debug(string.Format("[session-id={0}] running ISession.Dispose()", base.SessionId));
        if (base.TransactionContext != null)
        {
            base.TransactionContext.ShouldCloseSessionOnDistributedTransactionCompleted = true;
        }
        else
        {
            this.Dispose(true);
        }
    }
}

private void Dispose(bool isDisposing)
{
    using (new SessionIdLoggingContext(base.SessionId))
    {
        if (!base.IsAlreadyDisposed)
        {
            SessionImpl.log.Debug(string.Format("[session-id={0}] executing real Dispose({1})", base.SessionId, isDisposing));
            if (isDisposing && !base.IsClosed)
            {
                this.Close();
            }
            base.IsAlreadyDisposed = true;
            GC.SuppressFinalize(this);
        }
    }
}

I wonder if my fix is good enough and if the session will be closed by the 
transaction context or only by the GC.

Original issue reported on code.google.com by christia...@gmail.com on 20 Dec 2012 at 10:31

Attachments:

• Tests.cs.patch
• StateSession.cs.patch

[GoogleCodeExporter]

hello,

The whole point of using the DataContext inside of a using block is to ensure 
that the session is closed and managed. What if you created the transaction 
inside of the DataContext. It would seem like a good thing to close/accept the 
transaction after a successful operation (save changes). Thoughts?

Original comment by bniemyjski on 20 Dec 2012 at 5:20

• Changed state: AwaitingInfo
• Added labels: Framework-NHibernate, Type-Defect

[GoogleCodeExporter]

Hello,

I agree that in simple cases a DataContext inside a using block with a DB
local transaction inside is the best choice.
However, if multiple transactional systems are participating in a
transaction, one needs an eventual distributed transaction with a scope
wider than one single DB:

using (scope = new TransactionScope())
{
  using (DataContext1) {...}
  using (DataContext2) {...}
  using (ContextOfOtherTransactionalSystem) {...}
  scope.Complete();
}

This 
post<http://blogs.msdn.com/b/mattwrock/archive/2010/12/25/getting-transactionsco
pe-to-play-nice-with-nhibernate.aspx>explains
how NHibernate can work with TransactionScope. (See also
here<http://stackoverflow.com/questions/646318/nhibernate-with-transactionscope>
.)
Since this pattern works well with NHibernate directly I believe it should
also work with PLINQO.
Remarks?

Original comment by christia...@gmail.com on 20 Dec 2012 at 7:35

[GoogleCodeExporter]

As I understand the NHibernate connection gets cleaned up by the
TransactionScope around it, even if one does not close the NHibernate
connection manually (as currently done in PLINQO).
(Closing the connection before the outer transaction can roll back causes
my exception.)

See also here:
http://thatextramile.be/blog/2010/05/avoiding-leaking-connections-with-nhibernat
e-and-transactionscope

As stated in that article, the explicit use of NHibernate transactions is
no longer required since v3.1 to prevent connection leaks. Therefore the
use case I designed in my unit test without inner transaction ought to work
too.

Original comment by christia...@gmail.com on 20 Dec 2012 at 11:06

[GoogleCodeExporter]

Hello,

If you look at the last blog post on the bottom. There solution is what we 
already do. They are creating a new transaction scope, then creating and 
closing a new session (inside the session is a transaction). I wouldn't think 
having multiple sessions open and close inside of a transaction scope would 
matter.

Can you create a tracker test that breaks this so we can both be looking at the 
same sample before changing code.

If we had to change any code in the long run. I think the best approach would 
be to pass a transaction or transactionscope to the datacontext constructor 
overload and see the transaction state. If it's ready to be committed/closed 
then close the context... Thoughts?

Original comment by bniemyjski on 21 Dec 2012 at 4:39

[GoogleCodeExporter]

Hello,

Have a look at my attached tracker tests of my first post. I believe they
use the pattern you refer to in that post. Of course I use the PLINQO
methods but under the hood the same should happen - except for the session
close which PLINQO does but not NHibernate.
My first post shows how NHibernate handles ambient transactions and session
closing. There is no need to pass a transactionscope into the datacontext
constructor because a) one could get the ambient transaction from
Transaction.Current if there is one and b) because NHibernate already
handles this nicely. IMO PLINQO should trust NHibernate with handling of
ambient transactions and session closing in its session.close (as shown in
my first post). Calling NHibernates session.dispose (which is already done
in the PLINQO session dispose) should be enough. I still believe my
proposed fix is ok.

Original comment by christia...@gmail.com on 22 Dec 2012 at 7:35

[GoogleCodeExporter]

Any news?

On Sat, Dec 22, 2012 at 8:34 AM, Christian Bang
<christianbang314@gmail.com>wrote:

Original comment by christia...@gmail.com on 16 Jan 2013 at 10:02

[GoogleCodeExporter]

Hello,

This has been fixed in the latest nightly build. Thanks for the patch and 
reporting this issue.

Original comment by bniemyjski on 17 Jan 2013 at 4:51

• Changed state: Fixed