2

I am working on a setup where a scalable WCF Service Component is connected to a single MS SQL Server Database. The RESTful service allows users to save data into the DB as well as get data from it.

Whilst implementing a class handling the database connections / methods, I started struggling with correctly reusing prepared SqlCommands and the connection. I read up on the MSDN about connection pooling as well as how to use SqlCommand and SqlParameter.

My initial version of the class looks like this:

public class SqlRepository : IDisposable
{
    private object syncRoot = new object();

    private SqlConnection connection;

    private SqlCommand saveDataCommand;
    private SqlCommand getDataCommand;

    public SqlRepository(string connectionString)
    {
        // establish sql connection
        connection = new SqlConnection(connectionString);
        connection.Open();

        // save data
        saveDataCommand = new SqlCommand("INSERT INTO Table (Operation, CustomerId, Data, DataId, CreationDate, ExpirationDate) VALUES (@Operation, @CustomerId, @Data, @DataId, @CreationDate, @ExpirationDate)", connection);
        saveDataCommand.Parameters.Add(new SqlParameter("Operation", SqlDbType.NVarChar, 20));
        saveDataCommand.Parameters.Add(new SqlParameter("CustomerId", SqlDbType.NVarChar, 50));
        saveDataCommand.Parameters.Add(new SqlParameter("Data", SqlDbType.NVarChar, 50));
        saveDataCommand.Parameters.Add(new SqlParameter("DataId", SqlDbType.NVarChar, 50));
        saveDataCommand.Parameters.Add(new SqlParameter("CreationDate", SqlDbType.DateTime));
        saveDataCommand.Parameters.Add(new SqlParameter("ExpirationDate", SqlDbType.DateTime));
        saveDataCommand.Prepare();

        // get data
        getTripCommand = new SqlCommand("SELECT TOP 1 Data FROM Table WHERE CustomerId = @CustomerId AND DataId = @DataId AND ExpirationDate > @ExpirationDate ORDER BY CreationDate DESC", connection);
        getTripCommand.Parameters.Add(new SqlParameter("CustomerId", SqlDbType.NVarChar, 50));
        getTripCommand.Parameters.Add(new SqlParameter("DataId", SqlDbType.NVarChar, 50));
        getTripCommand.Parameters.Add(new SqlParameter("ExpirationDate", SqlDbType.DateTime));
        getTripCommand.Prepare();
    }

    public void SaveData(string customerId, string dataId, string operation, string data, DateTime expirationDate)
    {
        lock (syncRoot)
        {
            saveDataCommand.Parameters["Operation"].Value = operation;
            saveDataCommand.Parameters["CustomerId"].Value = customerId;
            saveDataCommand.Parameters["CreationDate"].Value = DateTime.UtcNow;
            saveDataCommand.Parameters["ExpirationDate"].Value = expirationDate;
            saveDataCommand.Parameters["Data"].Value = data;
            saveDataCommand.Parameters["DataId"].Value = dataId;

            saveDataCommand.ExecuteNonQuery();
        }
    }

    public string GetData(string customerId, string dataId)
    {
        lock (syncRoot)
        {
            getDataCommand.Parameters["CustomerId"].Value = customerId;
            getDataCommand.Parameters["DataId"].Value = dataId;
            getDataCommand.Parameters["ExpirationDate"].Value = DateTime.UtcNow;

            using (var reader = getDataCommand.ExecuteReader())
            {
                if (reader.Read())
                {
                    string data = reader.GetFieldValue<string>(0);
                    return data;
                }
                else
                {
                    return null;
                }
            }
        }
    }

    public void Dispose()
    {
        try
        {
            if (connection != null)
            {
                connection.Close();
                connection.Dispose();
            }

            DisposeCommand(saveDataCommand);
            DisposeCommand(getDataCommand);
        }
        catch { }
    }

    private void DisposeCommand(SqlCommand command)
    {
        try
        {
            command.Dispose();
        }
        catch (Exception)
        {
        }
    }
}

There are several aspects important to know:

  • I am using SqlCommand.Prepare() to speed up the process of executing the command
  • Reusing the commands avoids creating new objects with every call to the GetData and SaveData methods, thus leading to no problem with the garbage collector
  • There is only one instance of the SqlRepository class, used by the WCF Service.
  • There are many many calls per minute to this service, so keeping a connection to the DB open is what I want.

Now I read up a bit more about connection pooling and the fact that it is highly recommended to use the SqlConnection object in a using statement to ensure disposal. To my understanding, the connection pooling technology takes care of leaving the connection open even though the Dispose() method of SqlConnection has been called by the using statement.

The way to use this would be to have a using(SqlConnection connection = new SqlConnection(connectionString)) inside the GetData and SaveData methods. However, then - at least to my intuition - I would need to create the SqlCommands inside the GetData / SaveData methods as well. Or not? I could not find any documentation on how to reuse the commands that way. Also wouldn't the call to SqlCommand.Prepare() be meaningless if I need to prepare a new command every time I get into the GetData / SaveData methods?

How do I properly implement the SqlRepository class? The way it is now I believe that if the connection breaks (maybe because the DB server goes down for a while and reboots), then the SqlRepository class will not automatically recover and be functioning. To my best knowledge this sort of failsave scenarios are handled in the pooling technology.

Thanks for ideas and feedback! Christian

5
  • Repositories are meant to work on top of a data-mapper (for example, an OR/M). Nov 19, 2015 at 10:23
  • I personally don't think you will notice anything by removing Prepare(). I also you prefere having a local DbConnection in the GetData(), SaveData() because of re-connection issues. As far as I know the ADO.Net Connection pooling is transparent. If you create a new DbConnection ADO.Net will get one from its internal pool and may re-use an "old" one. Personally I would not over-optimize here except you really encounter problems. To test this you may want to write some unit-tests or test-app and simply try it.
    – Marc
    Nov 19, 2015 at 10:47
  • There is zero benefit from re-using the SqlCommand object in the way that you are doing. If anything it is worse than simply creating a new object for each execution. You certainly should not be opening the SqlConnection in the constructor and leaving it open until the repository is disposed! Keep the connection open for as little time as possible.
    – GarethD
    Nov 19, 2015 at 10:49
  • The most important issue with this code is, it is causing synchronized database access.
    – Oguz Ozgul
    Nov 19, 2015 at 10:50
  • alright, thanks for the input. I will rewrite this using the connection pooling and creating the SqlCommands and parameters inside the methods. Thanks
    – Christian
    Nov 19, 2015 at 10:55

1 Answer 1

2

Do not reuse the SqlCommand instances.

You are synchronizing database access.

With your implementation, you are re-using a small object (which is no problem for the GC even if there are thousands) in exchange of concurrent DB operations.

  1. Remove the synchronization locks.
  2. Create new instances of SqlCommands for each database operation.
  3. Do not call Prepare. Prepare speeds up db operations, but after executing ExecuteReader() on a SqlCommand with CommandType = Text and with non-zero number of parameters, the command is unprepared internally.
1
  • Thanks, I'll do that instead.
    – Christian
    Nov 19, 2015 at 10:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.