Transactional Problems Afterword - Is this elegant?

Transactional Problems Afterword - Is this elegant?

Old forum URL: forums.lhotka.net/forums/t/4532.aspx


Patrick.Roeper posted on Thursday, March 20, 2008

The past couple of days I have been hit pretty hard with sorting out bugs from using the Transactional attribute on my dataportal methods. Everything was due to instantiating more than one database connection within the lifetime of a transaction. I've been trying to work out an elegant solution that is reusable and leverages features of the framework/C# so I would like to hear suggestions on how to improve what I have come up with already.

The place where everything starting is when I was making my accounting system more tolerant to failure. Example: When a user receives a payment in the system, I need to save the payment, create a new journal entry, update any credits that were applied, create any credits if the payment was an over payment, update all invoice balances to reflect the payment, ... the list continues. My way of handling this was to make an InvoicePaymentPoster command object who's responsibility was to make sure each of these items happened. I wrapped the DataPortal_Execute with a [Transaction] so that if any part of the entire process failed, nothing would ever be committed to the database.

Each one of these different objects I am working with from within DataPortal_Execute is a root object of its own, so during a fetch/update in any of these objects I am instantiating a new SqlConnection object which is where everything hit the fan. I read some posts on the forums and I saw Rocky mention the LocalContext object and how it could be used to share objects in the AppDomain. The next challenge was to come up with something that would fix my problems now, be elegant, efficient, and be something I could possible put into my codegen templates. So here is what I came up with:

    public static class Database
    {
        public static string Accounting
        {
            get { return ConfigurationManager.ConnectionStrings["Accounting"].ConnectionString; }
        }

        private static bool NewConnection(string connectionString, out SqlConnection cn)
        {
            if (ApplicationContext.LocalContext.Contains(connectionString))
            {
                cn = (SqlConnection)ApplicationContext.LocalContext[connectionString];
                return false;
            }
            else
            {
                cn = new SqlConnection(connectionString);
                return true;
            }
        }

        public delegate void CommandHandler(SqlConnection cn);

        public static void CallCommand(string connectionString, CommandHandler commandMethod)
        {
            SqlConnection cn;

            if (NewConnection(connectionString, out cn))
            {
                using (cn)
                {
                    cn.Open();

                    ApplicationContext.LocalContext.Add(connectionString, cn);
                    try { commandMethod(cn); }
                    finally { ApplicationContext.LocalContext.Remove(connectionString); }
                }
            }
            else
                commandMethod(cn);
        }
    }

This makes all of my root level DataPortal_XYZ methods look something like...

        [Transactional(TransactionalTypes.TransactionScope)]
        protected override void DataPortal_Update()
        {
            Database.CallCommand(Database.Accounting, CommandUpdate);
        }

        private void CommandUpdate(SqlConnection cn)
        {
            if (base.IsDirty)
                ExecuteUpdate(cn, null);

            //update child object(s)
            UpdateChildren(cn);
        }

Everything is working in my application now; database connections are being shared across the multiple root objects, the code is repeatable and easy to implement for anyone after me, and I can put this into my codegen templates. Also, when it comes to fetching objects, I had to deal with the extra filter criteria parameter so I did the following:

        public delegate void CommandHandlerWithCriteria<T>(SqlConnection cn, T criteria);

        public static void CallCommand<T>(string connectionString, CommandHandlerWithCriteria<T> commandMethod, T criteria)
        {
            SqlConnection cn;

            if (NewConnection(connectionString, out cn))
            {
                using (cn)
                {
                    cn.Open();

                    ApplicationContext.LocalContext.Add(connectionString, cn);
                    try { commandMethod(cn, criteria); }
                    finally { ApplicationContext.LocalContext.Remove(connectionString); }
                }
            }
            else
                commandMethod(cn, criteria);
        }

And the root implementation looks like....

private void DataPortal_Fetch(Criteria criteria)
        {
            Database.CallCommand(Database.Accounting, ExecuteFetch, criteria);
        }

        private void ExecuteFetch(SqlConnection cn, Criteria criteria)
        {
            using (SqlCommand cm = cn.CreateCommand())
            {
                ....

So I am only a junior developer (been out of college for all of 2 months now Big Smile [:D]) and I am sure there is some code smell I haven't gotten a wiff of. I would appreciate any insight to potential problems this implementation could cause as well as suggestions on ways to make it better (e.g. standards I am not adhering to). When I make the jump to .NET 3.5 I plan on using the Action<T> since it supports multiple parameters in .NET 3.5; that would allow me to drop my delegates and make the code a little cleaner.

Thanks!

RockfordLhotka replied on Friday, March 21, 2008

Look at the Csla.Data.ConnectionManager class in CSLA 3.5 - it is designed to help in this situation by automatically reusing an already-open connection if there is one. That class should back-port all the way to .NET 2.0 will little or no change.

The one thing though, is that ultimately you do have ONE root object for the transaction - that's the single option where the data portal calls your DataPortal_XYZ method. That method MUST keep the connection open for the life of the transaction - which again is easy with ConnectionManager:

protected override void DataPortal_Update()
{
  using (ConnectionManager<SqlConnection> ctx =
            ConnectionManager<SqlConnection>.GetManager("mydatabase"))
  {
    // the database is now ctx.Connection, which is open and ready to go
    // do all database work (and call other objects) here
  }
}

Each object (root or child) should use this same pattern with the using block and a ConnectionManager. This using statement will open the connection if it isn't open, or will reuse an already open one if it is open.

Patrick.Roeper replied on Friday, March 21, 2008

Rocky,

I just read over the implementation of your connection manager class and I am going to implement this in my solution in place of what I came up with above. I did have a couple questions however...

public class ConnectionManager<C> : IDisposable where C : IDbConnection, new()

What does the "new()" portion of the line above do/mean?

Second, where would using GetCurrentManager be prefered over using GetManager? The only difference is GetCurrentManager throws an exception if you try to access a shared database connection that doesnt exist; where would a developer want to take advantage of that rather than initializing a new connection?

Last, I am trying to think further down the road to new data access technologies that microsoft has on their product roadmap, specifically the entity framework. I haven't read much on the actual implementation of a DAL using the entity framework, but do you expect to be able to control the database connection that the entity framework would use to talk to the database? If not, couldn't this transactional problem resurface?

Thanks as always.
Patrick

tmg4340 replied on Friday, March 21, 2008

I can answer one part of this...

The "new()" portion is another generic constraint.  It means that the type that "C" represents must have a public parameterless constructor.  That way you are guaranteed the ability to create instances of that type.  Since the ConnectionManager class wraps a database connection, Rocky's code needs to be able to create an instance of the connection class.  "IDbConnection" is an interface, and thus does not guarantee the ability to create an instance of the type by itself.

(FWIW, switching it to use the "DbConnection" base class wouldn't do it either - its constructor is protected.  So you'd still need the "new()" constraint.)

HTH

- Scott

RockfordLhotka replied on Friday, March 21, 2008

GetCurrentManager was an experimental concept that won’t be in 3.5 – so the answer is to not use it at all :)

 

Rocky

Copyright (c) Marimer LLC