7
\$\begingroup\$

I have the following method that is responsible for retrieving an Item from the database. There will be multiple threads instantiating this class, so I want to make sure that the operation will be thread-safe. As a result, I have added a lock around it, however since class only contains instance variables (and no static variables), is adding a lock necessary for this scenario?



using System;
using System.Collections.Generic;
using System.Data;
using System.Data.SqlClient;

namespace My.Library
{
    public class MyDataProvider : IMyDataProvider
    {
        private object sync = new object();
        private string connectionString;

        public MyDataProvider(string connString)
        {
            connectionString = connString;
        }
        public Item GetItem(Guid itemId)
        {
            lock (sync)
            {
                Item item = new Item();
                string sqlQuery = @"SELECT ItemId, ItemName FROM Item WHERE ItemId = @itemId";

                using SqlConnection connection = new SqlConnection(connectionString);
                SqlCommand command = new SqlCommand(sqlQuery, connection);
                command.Parameters.AddWithValue("@itemId", itemId);
                connection.Open();

                SqlDataReader reader = command.ExecuteReader();

                while (reader.Read())
                {
                    item.ItemId = reader["ItemId"] == DBNull.Value ? Guid.Empty : (Guid)reader["ItemId"];
                    item.ItemName = reader["ItemName"] == DBNull.Value ? null : reader["ItemName"];
                }

                return item;
            }
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Why do you think locking is needed? What resource do you want to protect? Which piece of code would require exclusive access? \$\endgroup\$ – Peter Csala Jul 23 at 12:31
8
\$\begingroup\$

According to information you provided it's not necessary to wrap it with lock.

Let's consider situation you described. You wrote:

There will be multiple threads instantiating this class

So there will be one instance for each thread. There's no synchronisation needed, because only one thread will be accessing your class at a time.

Other scenarios:

  1. Single instance of MyDataProvider class is shared between multiple threads. What does it mean? GetItem() method could be invoked by multiple threads at the same time. In that case you'll need synchronization IF your class have any state that (private field for example) could be accessed and modified from GetItem() method. You don't have such situation, so synchronization is not needed. Also you get connection from '[POOL]' each time anyone is invoking GetItem() instance method.
  2. Single instance of MyDataProvider class is shared between multiple threads AND sqlconnection is a state in it. Then you would need to synchronize it.

Your implementation should be fine.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ So if a single instance of MyDataProvider is not shared across multiple threads, I can remove the lock, correct? \$\endgroup\$ – BKS Jul 23 at 10:25
  • \$\begingroup\$ Yes. Even if it will be shared you don't modify your state (connectionString field) so everything should be fine. \$\endgroup\$ – Karol Miszczyk Jul 23 at 10:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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