Is it safe to make SqlConnection in a DAL class ThreadLocal?

Question

I have the following DAL class hierarchy (partially shown), which I use for abstracting data access to the database. I would like to use it in a thread-safe manner:

public class DbAdapter : IDbAdapter, IDisposable
{
    private SqlConnection _conn;
    private readonly string _connString;

    protected DbAdapter(string connString)
    {
        if (string.IsNullOrWhiteSpace(connString))
            throw new ArgumentException("Value cannot be null, empty or whitespace", nameof(connString));
        _connString = connString;
    }

    public void Dispose()
    {
        CloseConnection();
    }

    public SqlConnection GetConnection()
    {

        if (_conn == null || _conn.State == ConnectionState.Closed)
            _conn = new SqlConnection(_connString);
        else if (_conn.State == ConnectionState.Broken)
        {
            _conn.Close();
            _conn.Open();
        }
        return _conn;
    }

    public void CloseConnection()
    {
        if (_conn != null && _conn.State == ConnectionState.Open)
            _conn.Close();
        _conn = null;
    }

    public SqlCommand GetCommand(string query, SqlTransaction transaction)
    {
        var cmd = new SqlCommand
        {
            Connection = transaction != null ? transaction.Connection : GetConnection()
        };
        if (transaction != null)
            cmd.Transaction = transaction;
        cmd.CommandType = CommandType.Text;
        cmd.CommandText = query;
        cmd.CommandTimeout = 500;
        return cmd;
    }
    
    // Omitted other methods
}

public class PersonDba : DbAdapter 
{
    //Omitted 

    public IList<Person> GetPersons(string whereClause, string orderClause, SqlTransaction transaction = null)
    {
        var query = "SELECT Id, Name, PersonId, Birthdate, Modified FROM Persons ";

        if (!string.IsNullOrWhiteSpace(whereClause))
            query += whereClause;
        if (!string.IsNullOrWhiteSpace(orderClause))
            query += orderClause;

        IList<Person> result = new List<Person>();

        var sqlCmd = GetCommand(query, transaction);
        using (var reader = sqlCmd.ExecuteReader(CommandBehavior.CloseConnection))
        {
            while (reader.Read())
            {
                var person = new Person
                {
                    Id = reader.GetInt32(reader.GetOrdinal("Id")),
                    PersonId = reader.GetInt32(reader.GetOrdinal("PersonId")),
                    Name = reader.GetString(reader.GetOrdinal("Name")),
                    Birthdate = reader.GetDateTime(reader.GetOrdinal("Birthdate")),
                    LastModified = reader.GetDateTime(reader.GetOrdinal("Modified"))
                };
                result.Add(person);
            }
        }
        return result;
    }
}

Typically, I inject a single instance of PersonDba into other classes, which contain multi-threaded code. In order to avoid locking around all access to that single instance within dependant code, I am thinking of making the SQLConnection DbAdapter._conn of type ThreadLocal<SqlConnection> (see ThreadLocal). Would that be enough to ensure using instances of this class is thread-safe?

Answer 1

Assuming that you want to keep open the possibility of making your DAL asynchronous in the future, by using the ExecuteReaderAsync instead of the ExecuteReader, then converting the field _conn of the class DbAdapter to ThreadLocal<SqlConnection> is not safe. The reason is that after awaiting the asynchronous request, the asynchronous workflow will most probably continue on another ThreadPool thread. So the wrong connection will be closed, and some other unrelated concurrent data access operation may be interrupted and aborted.