Безопасно ли создавать SqlConnection в классе DAL ThreadLocal?

У меня есть следующая иерархия классов DAL (показана частично), которую я использую для абстрагирования доступа к данным в базе данных. Я хотел бы использовать его потокобезопасным способом:

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

Обычно я внедряю один экземпляр PersonDba в другие классы, содержащие многопоточный код. Чтобы избежать блокировки всего доступа к этому единственному экземпляру в зависимом коде, я думаю сделать SQLConnection DbAdapter._conn типа ThreadLocal<SqlConnection> (см. ThreadLocal). Будет ли этого достаточно, чтобы использование экземпляров этого класса было потокобезопасным?


person Bahaa    schedule 24.09.2020    source источник
comment
Это плохая абстракция, потому что вы вынуждаете потребителей создавать предложения where в виде строк, а это означает, что они не могут использовать наиболее подходящий способ избежать проблем с внедрением SQL — параметры.   -  person Damien_The_Unbeliever    schedule 24.09.2020
comment
Это правда. Это проблема устаревшего кода, которой я управляю на верхних уровнях. Тем не менее суть вопроса не в этом.   -  person Bahaa    schedule 24.09.2020
comment
comment
Вопрос, который вы связали, касается чего-то другого. В моем вопросе я НЕ хочу делиться SqlConnection между потоками. Наоборот, я хочу сделать его ThreadLocal, что дает каждому потоку собственное значение.   -  person Bahaa    schedule 24.09.2020
comment
У вас большая проблема с дизайном. Каждый поток должен иметь собственный объект репо и последующее подключение к базе данных. Ваш PersonDba и базовый класс - странный способ сделать шаблон репо   -  person MickyD    schedule 24.09.2020
comment
Думал и об этом. Например, потребуется рефакторинг, чтобы внедрить вместо этого PersonDbaFactory, который дает каждому порожденному потоку свой экземпляр. Но, еще раз, я специально спрашиваю, решит ли проблему создание переменной экземпляра _conn ThreadLocal.   -  person Bahaa    schedule 24.09.2020
comment
Вот почему это комментарии, а не ответы   -  person MickyD    schedule 24.09.2020
comment
Можно ли использовать ExecuteReaderAsync вместо ExecuteReader в будущем, или можно предположить, что ваш уровень доступа к данным останется синхронным навсегда?   -  person Theodor Zoulias    schedule 24.09.2020
comment
Да, @TheodorZoulias, это возможно   -  person Bahaa    schedule 24.09.2020
comment
Это в основном субъективно, но я бы настоятельно не советовал бы этого, не в последнюю очередь потому, что это вообще не будет работать с async, где вы можете прыгать повсюду с точки зрения потоков. Хорошо, вы можете обойти это, используя async-local или контекст выполнения для хранения внешних данных, но это будет все равно очень плохой идеей. IMO: передайте его явно, как и в случае с транзакцией (на самом деле, поскольку транзакция и соединение тесно связаны, их передача разными способами должна быть огромным красным флагом)   -  person Marc Gravell    schedule 24.09.2020
comment
Конечно, вы можете сделать его локальным для потока. Настоятельно подумайте о том, чтобы стиснуть зубы и провести рефакторинг этого материала — сначала можно было бы заставить CloseConnection/GetCommand принимать его в качестве параметра и начать переписывать вызывающие объекты вверх по цепочке, чтобы передавать полученное соединение. Затем это можно легко расширить, чтобы они использовали правильные шаблоны using. Этот дизайн уже сломан; добавление локального хранилища потока только сделает вещи более запутанными и ненадежными, а не менее.   -  person Jeroen Mostert    schedule 24.09.2020
comment
Примечание: здесь вы делаете много работы, которую такие инструменты, как Dapper, могли бы сделать для вас гораздо проще; также - предложение where/order-by в виде строки просто запрашивает SQL-инъекцию   -  person Marc Gravell    schedule 24.09.2020


Ответы (1)


Предполагая, что вы хотите сохранить возможность сделать DAL асинхронным в будущем, используя ExecuteReaderAsync вместо ExecuteReader, то преобразование поля _conn класса DbAdapter в ThreadLocal<SqlConnection> небезопасно. Причина в том, что после ожидания асинхронного запроса асинхронный рабочий процесс, скорее всего, продолжится в другом потоке ThreadPool. Таким образом, неправильное соединение будет закрыто, и некоторые другие несвязанные параллельные операции доступа к данным могут быть прерваны и прерваны.

person Theodor Zoulias    schedule 24.09.2020
comment
Я рассматриваю переменную нестатическую ThreadLocal‹SqlTransaction› _conn. Кроме того, в настоящее время код является синхронным, и он мог бы оставаться таковым, если можно гарантировать потокобезопасное выполнение в потребляющих классах. - person Bahaa; 24.09.2020
comment
@Bahaa Если вас устраивает постоянное ограничение синхронной модели, то ThreadLocal<SqlConnection> в безопасности. Да, делать его статическим не имеет смысла, потому что вы можете захотеть поддерживать несколько соединений на поток для разных баз данных. Я обновлю свой ответ. - person Theodor Zoulias; 24.09.2020