views:

334

answers:

5

Many examples out there advocate explicit rollback of database transactions, along the lines of:

using (var transaction = ...)
{
    try
    {
        // do some reading and/or writing here

        transaction.Commit();
    }
    catch (SqlException ex)
    {
        // explicit rollback
        transaction.Rollback();
    }
}

However, I tend to do this:

using (var transaction = ...)
{
    // do some reading and/or writing here

    transaction.Commit();
}

When an exception occurs, I'm just relying on the implicit rolling back of transactions that aren't committed.

Is there any problem relying on this implicit behavior? Does anyone have a convincing reason why I shouldn't be doing it this way?

A: 

I guess it could be answered that the first approach is more readable for someone maintaining your code. The explicit nature of the coding makes the objectives clear and quickly. While the implicit rollback is clear to you, and probably anyone who has more than a passing knowledge of the transaction handling, it may not be to others. That said, a few comments would quickly rectify that. The only concern is if the implicit rollback is not a documented feature of the object.

So, I'd say, providing you comment the operation and you can rely on the implicit action, then there's not a good reason to go for the explicit approach.

Lazarus
+4  A: 

No, its not specifically needed, however I can think of 2 reasons why it might be a good idea:

  • Clarity

Some might argue that using transaction.Rollback() makes it clearer under what circumstances the transaction will not be committed.

  • Releasing locks

When dealing with transactions it is important to realise the certain locks will only be released when the transaction is rolled back or committed. If you are using the using statement then the transaction will be rolled back when the transaction is disposed of, however if for some reason you need to do some error handling inside the using block, it may be advantageous to rollback the transaction (removing the locks) before performing complex / time consuming error handling.

Kragen
A: 

Most properly written ADO.NET connection will rollback transactions not explicitly committed. So it's not strictly necessary.

The main benefit I see of an explicit Rollback() call it the ability to set a breakpoint there and then inspect either the connection or the database to see what was going on. It's also clearer to future maintainers of the code what happens under different execution paths.

LBushkin
A: 

As long as the transaction is totally self-contained in the using() block, you are good to go. Problems can and do arise, though, if someone if you are passed an existing transaction object from a caller. But that's a different scenario...

Dave Markle
A: 

I see two problems with your usage:

  1. You're relying on the transaction's Dispose() method to rollback an uncommitted transaction, creating a dependency on a third-party's implementation of IDisposable. It's a negligible risk in this case of course but not a good practice.
  2. You are missing the opportunity to log and/or handle the exception. Also, I would catch Exception not SqlException.
Jamie Ide
Exception management and logging occurs further up in my stack. The first sample is not mine - it's what you typically find as example transaction management code.
Kent Boogaart
IMO, logging should always occur as close to the exception as possible. With transactions there's always the possibility of an exception so there should always be a try..catch block.
Jamie Ide
Well, that's your opinion and you're entitled to it :)
Kent Boogaart