List.Add() thread safety

I understand that in general a List is not thread safe, however is there anything wrong with simply adding items into a list if the threads never perform any other operations on the list (such as traversing it)?

Example:

List<object> list = new List<object>();
Parallel.ForEach(transactions, tran =>
{
	list.Add(new object());
});

Behind the scenes lots of things happen, including reallocating buffers and copying elements. That code will cause danger. Very simply, there are no atomic operations when adding to a list, at the least the "Length" property needs to be updates, and item needs to be put in at the right location, and (if there's a separate variable) the index needs to be updated. Multiple threads can trample over each other. And if a grow is required then there is lots more going on. If something is writing to a list nothing else should be reading or writing to it.

In .NET 4.0 we have concurrent collections, which are handily threadsafe and don't require locks.

You current approach is not thread-safe - I would suggest avoiding this altogether - since you basically do a data transformation PLINQ might be a better approach ( I know this is a simplified example but in the end you are projecting each transaction into another "state" object).

List<object> list = transactions.AsParallel()
                                .Select( tran => new object())
                                .ToList();

I solved my problem using ConcurrentBag<T> instead of List<T> like this:

ConcurrentBag<object> list = new ConcurrentBag<object>();
Parallel.ForEach(transactions, tran =>
{
    list.Add(new object());
});

If you want to use List.add from multiple threads and do not care about the ordering, then you probably do not need the indexing ability of a List anyway, and should use some of the available concurrent collections instead.

If you ignore this advice and only do add, you could make add thread safe but in unpredictable order like this:

private Object someListLock = new Object(); // only once

...

lock (someListLock)
{
    someList.Add(item);
}

If you accept this unpredictable ordering, chances are that you as mentioned earlier do not need a collection that can do indexing as in someList[i].

It's not an unreasonable thing to ask. There are cases where methods which can cause thread-safety issues in combination with other methods are safe if they are the only method called.

However, this clearly isn't a case of it, when you consider the code shown in reflector:

public void Add(T item)
{
    if (this._size == this._items.Length)
    {
        this.EnsureCapacity(this._size + 1);
    }
    this._items[this._size++] = item;
    this._version++;
}

Even if EnsureCapacity was in itself threadsafe (and it most certainly is not), the above code is clearly not going to be threadsafe, considering the possibility of simultaneous calls to the increment operator causing mis-writes.

Either lock, use ConcurrentList, or perhaps use a lock-free queue as the place multiple threads write to, and the read from it - either directly or by filling a list with it - after they have done their work (I'm assuming that multiple simultaneous writes followed by single-threaded reading is your pattern here, judging from your question, as otherwise I can't see how the condition where Add is the only method called could be of any use).

This would cause problems, as the List is built over an array and is not thread safe you might get index out of bounds exception or some values overriding other values, depending on where the threads are. Basically, don't do it.

There are multiple potential problem... Just don't. If you need a thread safe collection, either use a lock or one of the System.Collections.Concurrent collections.

>Is there anything wrong with simply adding items into a list if the threads never perform any other operations on the list?

Short answer: yes.

Long answer: run the program below.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;

class Program
{
	readonly List<int> l = new List<int>();
	const int amount = 1000;
	int toFinish = amount;
	readonly AutoResetEvent are = new AutoResetEvent(false);

	static void Main()
	{
		new Program().Run();
	}

	void Run()
	{
		for (int i = 0; i < amount; i++)
			new Thread(AddTol).Start(i);

		are.WaitOne();

		if (l.Count != amount ||
	        l.Distinct().Count() != amount ||
            l.Min() < 0 ||
	        l.Max() >= amount)
		    throw new Exception("omg corrupted data");

		Console.WriteLine("All good");
		Console.ReadKey();
	}

	void AddTol(object o)
	{
        // uncomment to fix
        // lock (l) 
		l.Add((int)o);

		int i = Interlocked.Decrement(ref toFinish);

		if (i == 0)
			are.Set();
	}
}

As others already said, you can use concurrent collections from the System.Collections.Concurrent namespace. If you can use one of those, this is preferred.

But if you really want a list which is just synchronized, you could look at the SynchronizedCollection<T>-Class in System.Collections.Generic.

Note that you had to include the System.ServiceModel assembly, which is also the reason why I don't like it so much. But sometimes I use it.

Even adding elements on different threads is not thread safe.

In C# 4.0 there are concurrent collections (see http://jiezhu0815.blogspot.com/2010/08/c-40-feature-1-concurrent-collections.html).