3

Resolved

Transactions held open on NHibernate exceptions (with fix)

description

I was working on some Orchard code today that was throwing up a NHibernate exception ("a different object with the same identifier value was already associated with the session").

While investigating, I found that (as expected) the transaction was not committing because of the above error. BUT - the transaction was not actually being terminated, i.e. the transaction was still open when I looked at SQL server.

I solved this by altering the Dispose() code in Orchard.Data.SessionLocator.

See inline comments:
public void Dispose() {
    if (_transaction != null) {
        try {
            if (!_cancelled) {
                Logger.Debug("Marking transaction as complete");
                _transaction.Commit();  // ## when this failed, the transaction was held open ##
            }
            else {
                Logger.Debug("Reverting operations from transaction");
                _transaction.Rollback();
            }

            _transaction.Dispose();  // ## i moved this line to finally block ##
            Logger.Debug("Transaction disposed"); // ## i moved this line to finally block
        }
        catch (Exception e) {
            Logger.Error(e, "Error while disposing the transaction.");
        }
        finally {
            _transaction.Dispose(); //   ## line moved from above to solve problem ##
            Logger.Debug("Transaction disposed"); // ## line moved from above ##
            _transaction = null;  
            _cancelled = false;
        }
    }
}

comments

CSADNT wrote Jan 17 at 5:59 PM

Shouldn't have you also move the transaction.Rollback() ?

Jimasp wrote Jan 17 at 6:26 PM

No. For a few reasons:
  1. The rollback is part of the logic ("if(cancelled)" path), so you wouldn't actually move it, rather you would add it. But, you wouldn't want to do that anyway because:
  2. The rollback could potentially also cause an exception (hence why its in the catch block).
  3. You wouldn't always want to rollback (remember finally ALWAYS executes, fail OR success).
  4. The transaction may have already committed or rolled back (see 3).
  5. It wouldn't be wise to try a rollback in the finally block anyway, as it might raise an exception itself, unless you put another t/c/f block around it, but that starts to get silly as these are supposed to be "exceptions" by name and nature! :-)

Jimasp wrote Jan 17 at 6:28 PM

p.s. don't forget to vote ;c)

CSADNT wrote Jan 17 at 7:22 PM

Sure, must be done correctly, so IF a transaction is running and if there is an error, the normal action is to rollback,isn't it ?

CSADNT wrote Jan 17 at 8:14 PM

Jimasp wrote Jan 18 at 8:39 AM

RE: "Sure, must be done correctly, so IF a transaction is running and if there is an error, the normal action is to rollback,isn't it ?"

Yes, I suppose it would technically be better to be explicit with a rollback and then a dispose. But that means you have to think - hmm... did we get in this finally block via a commit error or a rollback error? because there is no point trying to rollback if it that already failed (or maybe you could have 2 try/catch blocks, but that complicates things as now you don't have a single finally block with dispose ) etc., etc.

i.e. it starts to become a code rewrite, which all seems a bit pointless when the dispose will rollback implicitly anyway and is nice and simple.

At any rate, my intention here is just to highlight the issue, and supply the basic fix. I don't really care about how the Orchard team fixes it, as long as they fix it! :c)

sebastienros wrote Mar 20 at 11:17 PM

Fixed in changeset e96c392479a385a200bc9512da96af638213ffd4

CSADNT wrote Mar 21 at 7:26 AM

if (_transaction != null) {
 try
 { 
    if (!_cancelled) 
    {
            try 
            {
                 Logger.Debug("Marking transaction as complete");
                 _transaction.Commit();  
             } 
             catch (Exception e1) 
            {
                   Logger.Error(e1, "Error while disposing the transaction  cancelled = {0}.",cancelled);
                   _transaction.Rollback();
             }
     }
     else {
            Logger.Debug("Reverting operations from transaction");
            _transaction.Rollback();
     }
    }
    catch (Exception e) 
    {
        Logger.Error(e, "Error while commit/rollback of transaction, cancelled = {0}.", cancelled);
    }
    finally 
    {
         try
        {
                _transaction.Dispose(); //   ## line moved from above to solve problem ##
        }
        catch (Exception e2) 
        {
              Logger.Error(e2, "Error while disposing the transaction cancelled = {0}.", cancelled);
        }
        Logger.Debug("Transaction disposed"); // ## line moved from above ##
        _transaction = null;  
        _cancelled = false;
    }

sebastienros wrote Mar 21 at 4:26 PM

throw new ArgumentNullException();
Take that !

CSADNT wrote Mar 21 at 5:08 PM

_cancelled ???

CSADNT wrote Mar 21 at 9:16 PM

Hard to follow... I guest it is not insulting :)

sebastienros wrote Mar 21 at 11:33 PM

no, code duels, love it

CSADNT wrote Mar 22 at 4:15 AM

I see, personnaly I like music

Jimasp wrote Mar 22 at 11:30 AM

I quite like Jint ;c)

CSADNT wrote Mar 22 at 10:48 PM

I still think the solution applied is partial...let's see how long it will stay

Jimasp wrote Apr 2 at 7:10 PM

i was merging down 1.7.3 today, and i noticed something else odd here:
                catch (Exception e) {
                    Logger.Error(e, "Error while disposing the transaction.");
                }
surely that should be:
                catch (Exception e) {
                    Logger.Error(e, "Error while disposing the transaction.");
                    throw;
                }
it's swallowing the transaction exception.

surely it should bubble up? if a db commit fails, i want the user to know about it !