Is this code actually thread-safe?

Question

I have a question regarding the following code segment which I found on Microsoft's C# tutorial web page. In the code they provide a demo of tasks. In the event handler they create a task which updates an unprotected collection.

Is this code thread-safe? In my opinion it's not. What is the best way to make this code thread-safe?

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)."); }

I disagree with the following statement

students.Add(student);

I think it should be protected by a lock.

at what MSDN page did you this? What in particular are you wondering is thread safe? The Add call? I'm actually not sure that this code would function correctly, since you are calling MessageBox.Show in a task. Those should run on the UI thread. — default
– default, Commented Jul 12, 2015 at 17:01
Don't use ArrayList it is leftover from .NET 1.1 days before generics exited. You should be using List<Student> instead. — Scott Chamberlain
– Scott Chamberlain, Commented Jul 12, 2015 at 17:10
The MessageBox.Show() is a thread-safe call (because it's static). The ArrayList.Add() is not. — Henk Holterman
– Henk Holterman, Commented Jul 12, 2015 at 17:59

Dzienny · Accepted Answer · 2015-07-12 21:19:45Z

Is this code actually thread-safe?

No, it's not.

According to the documentation the ArrayList instance doesn't support concurrent modification, unless it's returned by the Synchronized method and it's not the case here.

Although it may not be obvious, the concurrent modification can happen in your example. The Task is queued to the ThreadPool and it will be run by some thread from this pool. If a user double clicks btnCreateStudent two tasks will be created, and since Thread.Sleep is not very precise and, anyway, the tasks don't have to be executed immediately (for example the ThreadPool queue may be full), thus two tasks, although scheduled at different times, may be executed at the same time.

What is the best way to make this code thread-safe?

It depends on what you mean by "the best".

The first solution would be to create the ArrayList with the Synchronized method.

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

But you will still have to use a lock for enumerating this list.

Enumerating through a collection is intrinsically not a thread-safe procedure. Even when a collection is synchronized, other threads can still modify the collection, which causes the enumerator to throw an exception. To guarantee thread safety during enumeration, you can either lock the collection during the entire enumeration or catch the exceptions resulting from changes made by other threads.

The other solution would be to use the List<T> and add locks wherever the collection is accessed. The List<T> is superior to the ArrayList because it contains the elements type, so you don't have to cast them on read, or you won't accidentally add an incompatible type to the collection.

If you don't care about the order of items, then you should use the ConcurrentBag<T>, which doesn't require any locking.

Felix K. · Accepted Answer · 2015-07-12 17:20:36Z

"Thread safety" depends highly on what you do outside of the seperate thread. If you don't touch students at all outside of your task during the runtime of the task, your code is thread safe.

If you use students outside during the task's lifetime you should synchronize the access. You can do this with a lock or other synchronization methods.

You can of course also use some of the concurrent collections.

I invoke the Task periodically . Each mouse click will create a separate Tread . What will lead to many competing threads

Collectives™ on Stack Overflow

Is this code actually thread-safe?

2 Answers 2

Comments

2 Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

2 Comments

Related