Is Lock necessary to make operation thread safe for this scenario

Question

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;
            }
        }
    }
}

Answer 1

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.