3

I've got a list full of structs, which I want to iterate through and alter concurrently.

The code is conceptually as follows:

Parallel.For(0, pointsList.Count(), i=> pointsList[i] = DoThing(pointsList[i]));

I'm neither adding to nor removing from the list, only accessing and mutating its items.

I imagine this is fine, but thought I should check: is this OK as is, or do I need to use Lock somewhere for fear that I mess up the list object??

Jack
  • 871
  • 1
  • 9
  • 17
  • [It is safe to perform multiple read operations on a `List`, but issues can occur if the collection is modified while it's being read.](https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1?view=netframework-4.7.2#thread-safety) – user4003407 Mar 26 '19 at 03:23
  • 1
    Possible duplicate of [Can a List be accessed by multiple threads?](https://stackoverflow.com/questions/4504335/can-a-listt-be-accessed-by-multiple-threads) or [Parallel.ForEach on List Thread Safety](https://stackoverflow.com/questions/11232167/parallel-foreach-on-listobject-thread-safety) – Lance U. Matthews Mar 26 '19 at 03:23
  • Hi @PetSerAl. This is indeed a relevant link, which I have already looked up. However, the page was not explicit though about exactly which operations would cause which issues. For example, I would expect adding, inserting, removing, and clearing a list all to cause issues, I would possibly think that simply accessing list elements is fine – Jack Mar 26 '19 at 03:28
  • Hi @BACON, thanks for the response: this question is different to the one you linked, which is premised on the idea that "the list will be locked during a changes", whereas I'm asking if this is really neccesary. – Jack Mar 26 '19 at 03:31
  • The link from @PetSerAl tells you all you need to know. Reading the list is fine, modifying it is not. – DavidG Mar 26 '19 at 03:36
  • *However, the page was not explicit though about exactly which operations would cause which issues.* Because this is not part of contract, but implementation details. Different implementations (.NET Framework, .NET Core, Mono or theirs different versions) can have different behavior in this cases. – user4003407 Mar 26 '19 at 03:39
  • @Jack True, but it's [one answer in particular](https://stackoverflow.com/a/4504360/150605) on that question that I feel also answers yours, which happens to be an older version of the text @PetSerAl linked to. It does make it clear that read operations are thread-safe, whereas operations that modify the `List<>` are not, though it leaves it to the reader to deduce which are which. I was going to say that using the `get` indexer like you are is not modifying the collection, but then you are using `set` as well, and while `set` does not modify the list (dimensions)...it kind of is, too. – Lance U. Matthews Mar 26 '19 at 03:39

1 Answers1

5

It is not guaranteed to be safe. Changing an element in the list increments the list's private _version field, which is how various threads can tell if the list has been modified. If any other thread attempts to enumerate the list or uses a method like List<T>.ForEach() then there could potentially be an issue. It sounds like it would be extremely rare, which isn't necessarily a good thing-- in fact, it makes for the most maddening type of defect.

If you want to be safe, create a new list instead of changing the existing one. This is a common "functional" approach to threading issues that avoids locks.

var newList = pointsList.AsParallel().Select( item => DoThing(item) );

When you're done, you can always replace the old list if you want.

pointsList = newList.ToList();
John Wu
  • 50,556
  • 8
  • 44
  • 80
  • 1
    Good point on it bumping the version number. I didn't expect it to do that but it does. The relevant source code is [here](https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Collections/Generic/List.cs#L158) / [here](https://referencesource.microsoft.com/#mscorlib/system/collections/generic/list.cs,184) / [here](https://github.com/Microsoft/referencesource/blob/master/mscorlib/system/collections/generic/list.cs#L184). – Lance U. Matthews Mar 26 '19 at 03:45
  • 1
    Oh yeah, it looks like incrementing _version in parallel could result in a dirty write. This is only relevant for when the list is being altered or used as an IEnumerable however, neither of which I am doing. Furthermore, if I did perform any of the above operations on pointsList later on, the residual value inside _version wouldn't actually have any detrimental affect. However, the thought process here is complex enough I think I'd want to save the next guy from having to think about it, and so I think I'll avoid assigning to the list in parallel, as suggested. – Jack Mar 26 '19 at 04:00
  • Thanks for the answer John. .AsParallel() is a nifty little extension method. – Jack Mar 26 '19 at 04:01
  • 1
    @Jack After further consideration, I reached the same conclusion as your 2nd sentence. The `set` indexer invalidates any enumeration operations...but why does that matter if you know there are none? It's like [using a `for` loop to remove list items](https://stackoverflow.com/a/17767186/150605): no, you can't simultaneously access the list on another thread, but having that limitation doesn't mean that loop is "unsafe". I suppose it boils down to if you know the list won't be accessed in problematic ways then there's no problem, otherwise taking the cautious approach isn't a bad thing, either. – Lance U. Matthews Mar 26 '19 at 19:38