Exception handling around the rollback of a SqlTransaction

Question

I have two stored procedures that I want execute wrapped in a transaction. For various reasons, I need to handle the transaction in my application code instead of within the database.

At the moment, my code looks like this:

try
{
    using (SqlConnection conn = Connection())
    {
        conn.Open();

        using (SqlTransaction sqlTrans = conn.BeginTransaction())
        {
            try
            {
                using (SqlCommand cmd1 = new SqlCommand("Stored_Proc_1", conn, sqlTrans))
                {
                    cmd1.CommandType = CommandType.StoredProcedure;
                    cmd1.ExecuteNonQuery();
                }

                using (SqlCommand cmd2 = new SqlCommand("Stored_Proc_2", conn, sqlTrans))
                {
                    cmd2.CommandType = CommandType.StoredProcedure;
                    cmd2.ExecuteNonQuery();
                }

                sqlTrans.Commit();
            }
            catch
            {
                    sqlTrans.Rollback();

                    throw;
            }

        }

        conn.Close();
    }
}

catch (SqlException ex)
{
  // exception handling and logging code here...
}

When one of the stored procs raises an error, the exception message I am seeing looks like:

Error message from raiserror within stored procedure.
Transaction count after EXECUTE indicates that a COMMIT or ROLLBACK TRANSACTION statement is missing. Previous count = 1, current count = 0.

Which makes sense, because at the first catch, the transaction has not been rolled back yet.

But I want a "clean" error (without the tran count message - I'm not interested in this because I am rolling back the transaction) for my exception handling code. Is there a way I can restructure my code to achieve this?

EDIT:

The basic structure of my stored procs looks like this:

create proc Stored_Proc_1
as

set nocount on

begin try
    begin transaction 

     raiserror('Error raised by Stored_Proc_1', 16, 1)  

    commit

end try
begin catch  
    if (@@trancount > 0) rollback 

    declare @ErrMsg nvarchar(4000), @ErrSeverity int, @ErrProc sysname, @ErrLine varchar(10)
    select @ErrMsg = ERROR_MESSAGE(), @ErrSeverity = ERROR_SEVERITY(), @ErrProc = ERROR_PROCEDURE(), @ErrLine = ERROR_LINE()

    -- log the error 
    -- sql logging code here...

    raiserror(@ErrMsg, @ErrSeverity, 1) 
end catch

UPDATE: I've taken the transaction handling out of my stored procedures and that seems to have solved the problem. Obviously I was doing it wrong - but I'd still like to know how to do it right. Is removing transactions from the stored procedures the best solution?

Answer 1

Well, the conn.Close() could go anyway - it'll get closed by the using (if you think about it, it is odd that we only Close() it after an exception).

Do either of your stored procedures do any transaction code inside themselves (that isn't being rolled back/committed)? It sounds like that is where the problem is...? If anything, the error message suggests to me that one of the stored procedures is doing a COMMIT even though it didn't start a transaction - perhaps due to the (incorrect) approach:

-- pseduo-TSQL
IF @@TRANCOUNT = 0 BEGIN TRAN
-- ...
IF @@TRANCOUNT > 0 COMMIT TRAN -- or maybe = 1

(if you do conditional transactions in TSQL, you should track (via a bool flag) whether you created the transaction - and only COMMIT if you did)

The other option is to use TransactionScope - easier to use (you don't need to set it against each command etc), but slightly less efficient:

using(TransactionScope tran = new TransactionScope()) {
    // create command, exec sp1, exec sp2 - without mentioning "tran" or
    // anything else transaction related

    tran.Complete();
}

(note there is no rollback etc; the Dispose() (via using) will do the rollback if it needs to.

Answer 2

I agree with Marc that the problem is likely to be within the stored procedures themselves. There's a quite interesting article outlining a few issues here.

Answer 3

Don't do transactions in your database/stored procedures if you do this in your application! This will most surely just create confusion. Pick a layer and stick to it. Make sure you have a nice normalised database and exceptions should percolate upwards.