Является ли этот код фактически потокобезопасным?

Question

У меня есть вопрос относительно следующего сегмента кода, который я нашел на веб-странице Microsoft C# tutorial. В коде они предоставляют демонстрацию задач. В обработчике событий они создают задачу, которая обновляет незащищенную коллекцию.

Этот код безопасен? По-моему, это не так. Каков наилучший способ сделать этот код потокобезопасным?

private ArrayList students = new ArrayList(); 
private void btnCreateStudent_Click(object sender, RoutedEventArgs e) 
{ 
    Student newStudent = new Student(); 
    newStudent.FirstName = txtFirstName.Text; 
    newStudent.LastName = txtLastName.Text; 
    newStudent.City = txtCity.Text; 
    ClearForm(); 
    Task task1 = new Task(() => AddToCollection(newStudent)); 
    task1.Start(); 
    ClearForm(); 
} 

private void AddToCollection(Student student) 
{ 
    Thread.Sleep(5000); 
    students.Add(student); 
    int count = students.Count; 
    MessageBox.Show("Student created successfully. Collection contains " + count.ToString() + " Student(s)."); 
} 

Я не согласен со следующим утверждением

students.Add(student); 

Я думаю, что она должна быть защищена замком.

Answer 1

«Защита резьбы« В зависимости от того, что вы делаете за пределами отдельной нити. Если вы не прикасаетесь к students во время выполнения задачи во время выполнения задачи, ваш код будет потокобезопасным.

Если вы используете students снаружи во время жизни задачи, вам необходимо синхронизировать доступ. Вы можете сделать это с помощью lock или other synchronization methods.

Конечно, Вы можете также использовать некоторые из concurrent collections.

Answer 2

Является ли этот код на самом деле потокобезопасной?

Нет, это не так.

В соответствии с documentation экземпляр ArrayList не поддерживает одновременную модификацию, если только он не возвращается методом Synchronized, и здесь это не так.

Хотя это может быть и не очевидно, одновременная модификация может произойти в вашем примере. Task поставлен в очередь на ThreadPool, и он будет управляться некоторым потоком из этого пула. Если пользователь дважды щелкает btnCreateStudent, будут созданы две задачи, а так как Thread.Sleep не очень точен и, во всяком случае, задачи не должны выполняться немедленно (например, очередь ThreadPool может быть заполнена), таким образом, две задачи, хотя и запланированы в разное время, может выполняться одновременно.

Каков наилучший способ сделать этот код потокобезопасным?

Это зависит от того, что вы подразумеваете под «лучшим».

Первым решением было бы создать ArrayList с помощью метода Synchronized.

private ArrayList students = ArrayList.Synchronized(new ArrayList()); 

Но вам все равно придется использовать блокировку для перечисления этого списка.

Перечисление через коллекцию по существу не является потокобезопасной процедурой . Даже когда коллекция синхронизирована, другие потоки могут по-прежнему изменять коллекцию, что заставляет перечислитель перебрасывать исключение .Чтобы гарантировать безопасность потока во время перечисления, вы можете либо заблокировать сбор во время всего перечисления, либо уловить исключения из изменений, внесенных другими потоками.

Другим решением было бы использовать List<T> и добавлять блокировки везде, где доступна коллекция. List<T> превосходит ArrayList, потому что он содержит тип элементов, поэтому вам не нужно бросать их на чтение, иначе вы случайно не добавите несовместимый тип в коллекцию.

Если вы не заботитесь о порядке вещей, то вам следует использовать ConcurrentBag<T>, который не требует блокировки.