Detecting a Dispose() from an exception inside using block

Question

I have the following code in my application:

using (var database = new Database()) {
    var poll = // Some database query code.

    foreach (Question question in poll.Questions) {
        foreach (Answer answer in question.Answers) {
            database.Remove(answer);
        }

        // This is a sample line  that simulate an error.
        throw new Exception("deu pau"); 

        database.Remove(question);
    }

    database.Remove(poll);
}

This code triggers the Database class Dispose() method as usual, and this method automatically commits the transaction to the database, but this leaves my database in an inconsistent state as the answers are erased but the question and the poll are not.

There is any way that I can detect in the Dispose() method that it being called because of an exception instead of regular end of the closing block, so I can automate the rollback?

I don´t want to manually add a try ... catch block, my objective is to use the using block as a logical safe transaction manager, so it commits to the database if the execution was clean or rollbacks if any exception occured.

Do you have some thoughts on that?

Answer 1

As others have said, your use of the Disposable pattern for this purpose is what is causing you the problems. If the pattern is working against you, then I would change the pattern. By making a commit the default behaviour of the using block, you are assuming that every use of a database results in a commit, which clearly is not the case - especially if an error occurs. An explicit commit, possibly combined with a try/catch block would work better.

However, if you really want to keep your use of the pattern as is, you can use:

bool isInException = Marshal.GetExceptionPointers() != IntPtr.Zero
                        || Marshal.GetExceptionCode() != 0;

in your Displose implementation to determine if an exception has been thrown (more details here).

Answer 2

All others already wrote what the correct design of your Database class should be, so I won't repeat that. However, I didn't see any explanation why what you want to is not possible. So here it goes.

What you want to do is detect, during the call to Dispose, that this method is called in the context of an exception. When you would be able to do this, developers won't have to call Commit explicitly. However, the problem here is that there is no way to reliable detect this in .NET. While there are mechanisms to query the last thrown error (like HttpServerUtility.GetLastError), these mechanisms are host specific (so ASP.NET has an other mechanism as a windows forms for instance). And while you could write an implementation for a specific host implementation, for instance an implementation that would only work under ASP.NET, there is another more important problem: what if your Database class is used, or created within the context of an exception? Here is an example:

try
{
    // do something that might fail
}
catch (Exception ex)
{
    using (var database = new Database())
    {
        // Log the exception to the database
        database.Add(ex);
    } 
}

When your Database class is used in the context of an Exception, as in the example above, how is your Dispose method supposed to know that it still must commit? I can think of ways to go around this, but it will quite fragile and error prone. To give an example.

During creation of the Database, you could check whether it is called in the context of an exception, and if that’s the case, store that exception. During the time Dispose is called, you check whether the last thrown exception differs from the cached exception. If it differs, you should rollback. If not, commit.

While this seems a nice solution, what about this code example?

var logger = new Database();
try
{
    // do something that might fail
}
catch (Exception ex)
{
    logger.Add(ex);
    logger.Dispose();
}

In the example you see that a Database instance is created before the try block. Therefore is can not correctly detect that it sould not roll back. While this might be a contrived example, it shows the difficulties you will face when trying to design your class in a way no explicit call to Commit is needed.

In the end you will be making your Database class hard to design, hard to maintain, and you'll never really get it right.

As all others already said, a design that needs an explicit Commit or Complete call, would be easier to implement, easier to get right, easier to maintain, and gives usage code that is more readable (for instance, because it looks what developers expect).

Last note, if you're worried about developers forgetting to call this Commit method: you can do some checking in the Dispose method to see whether it is called without Commit is called and write to the console or set a breakpoint while debugging. Coding such a solution would still be much easier than trying to get rid of the Commit at all.

Update: Adrian wrote an intersting alternative to using HttpServerUtility.GetLastError. As Adrian notes, you can use Marshal.GetExceptionPointers() which is a generic way that would work on most hosts. Please note that this solution has the same drawbacks explained above and that calling the Marshal class is only possible in full trust

Answer 3

Look at the design for TransactionScope in System.Transactions. Their method requires you to call Complete() on the transaction scope to commit the transaction. I would consider designing your Database class to follow the same pattern:

using (var db = new Database()) 
{
   ... // Do some work
   db.Commit();
}

You might want to introduce the concept of transactions outside of your Database object though. What happens if consumers want to use your class and do not want to use transactions and have everything auto commit?

Answer 4

In short: I think that's impossible, BUT

What you can do is to set a flag on your Database class with default value "false" (it's not good to go) and on the last line inside using block you call a method that sets it to "true", then in the Dispose() method you may check whether the flag "has exception" or not.

using (var db = new Database())
{
    // Do stuff

    db.Commit(); // Just set the flag to "true" (it's good to go)
}

And the database class

public class Database
{
    // Your stuff

    private bool clean = false;

    public void Commit()
    {
        this.clean = true;
    }

    public void Dispose()
    {
        if (this.clean == true)
            CommitToDatabase();
        else
            Rollback();
    }
}

Answer 5

Sound like you need TransactionScope http://msdn.microsoft.com/en-us/library/system.transactions.transactionscope.aspx

Answer 6

You should wrap the contents of your using block in a try/catch and roll back the transaction in the catch block:

using (var database = new Database()) try
{
    var poll = // Some database query code.

    foreach (Question question in poll.Questions) {
        foreach (Answer answer in question.Answers) {
            database.Remove(answer);
        }

        // This is a sample line  that simulate an error.
        throw new Exception("deu pau"); 

        database.Remove(question);
    }

    database.Remove(poll);
}
catch( /*...Expected exception type here */ )
{
    database.Rollback();
}

Answer 7

As Anthony points out above, the problem is a fault in your use of the using clause in this scenario. The IDisposable paradigm is meant to ensure that an object's resources are cleaned up regardless of the outcome of a scenario (thus why an exception, return, or other event that leaves the using block still triggers the Dispose method). But you've repurposed it to mean something different, to commit the transaction.

My suggestion would be as others have stated and use the same paradigm as TransactionScope. The developer should have to explicitly call a Commit or similar method at the end of the transaction (before the using block closes) to explicitly say that the transaction is good and ready to be committed. Thus, if an exception causes execution to leave the using block, the Dispose method in this case can do a Rollback instead. This still fits in the paradigm, since doing a Rollback would be a way of "cleaning up" the Database object so that it is not left an invalid state.

This shift in design would also make it much easier to do what you want to do, since you won't be fighting the design of .NET to try and "detect" an exception.

Answer 8

You could inherit from the Database class and then override the Dispose() method (making sure to close the db resources), this could then raise a custom event to which you can subscribe in your code.