Analyzer to recommend ThreadSafe collections in parallel context.

[gfs]

Describe the problem you are trying to solve

A common pattern is to run parallel processing and to produce a set of data. The default collection types like List and HashSet are not threadsafe and you may get unexpected behavior (like nulls being added to the collection) if you modify them from multiple threads at the same time.

Describe suggestions on how to achieve the rule

I think there should be an analyzer that detects when you are using the non thread safe collections in the context of the built in parallelization mechanisms (ForAll or Parallel) and can swap out the collection you are using for a more appropriate collection.

Good Examples:

• Replace List with ConcurrentBag (add operations will not need to be changed, indexing operations would, which adds complexity to this)
• Replace Dictionary with ConcurrentDictionary

Other examples:

• Replace HashSet with ConcurrentDictionary<T,bool> (this is a workaround for there not being a threadsafe HashSet. The bool value is then ignored but you get threadsafety in a HashSet like structure.)

Additional context

The following set of tests demonstrates this issue in action along with potential rewrites.

namespace NonThreadSafeCollections
{
    using Microsoft.VisualStudio.TestPlatform.ObjectModel;
    using Microsoft.VisualStudio.TestTools.UnitTesting;
    using System;
    using System.Collections.Concurrent;
    using System.Collections.Generic;
    using System.Linq;
    using System.Security.Cryptography;

    [TestClass]
    public class NonThreadSafeCollections
    {
        static List<TestObject> generatedObjects = new List<TestObject>();
        
        [TestMethod]
        public void ParallelListNested()
        {
            Dictionary<(char, char), List<TestObject>> testObjects = new();
            generatedObjects.AsParallel().ForAll(x => {
                if (!testObjects.ContainsKey((x.FieldOne[0], x.FieldTwo[0])))
                {
                    testObjects[(x.FieldOne[0], x.FieldTwo[0])] = new List<TestObject>();
                }
                testObjects[(x.FieldOne[0], x.FieldTwo[0])].Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Values.Select(x => x.Count).Sum());
            Assert.IsFalse(testObjects.Any(x => x.Value.Any(y => y is null)));
        }

        [TestMethod]
        public void ParallelListNested_Fixed()
        {
            ConcurrentDictionary<(char, char), ConcurrentBag<TestObject>> testObjects = new();
            foreach(var character1 in chars)
            {
                foreach(var character2 in chars)
                {
                    testObjects[(character1, character2)] = new ConcurrentBag<TestObject>();
                }
            }
            generatedObjects.AsParallel().ForAll(x => {
                testObjects[(x.FieldOne[0], x.FieldTwo[0])].Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Values.Select(x => x.Count).Sum());
            Assert.IsFalse(testObjects.Any(x => x.Value.Any(y => y is null)));
        }

        [TestMethod]
        public void SingleThreadedListNested()
        {
            Dictionary<(char, char), List<TestObject>> testObjects = new();
            generatedObjects.ForEach(x => {
                if (!testObjects.ContainsKey((x.FieldOne[0], x.FieldTwo[0])))
                {
                    testObjects[(x.FieldOne[0], x.FieldTwo[0])] = new List<TestObject>();
                }
                testObjects[(x.FieldOne[0], x.FieldTwo[0])].Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Values.Select(x => x.Count).Sum());
            Assert.IsFalse(testObjects.Any(x => x.Value.Any(y => y is null)));
        }

        [TestMethod]
        public void ParallelList()
        {
            List<TestObject> testObjects = new();
            generatedObjects.AsParallel().ForAll(x => {
                testObjects.Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Count);
            Assert.IsFalse(testObjects.Any(x => x is null));
        }

        [TestMethod]
        public void ParallelList_Fixed()
        {
            ConcurrentBag<TestObject> testObjects = new();
            generatedObjects.AsParallel().ForAll(x => {
                testObjects.Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Count);
            Assert.IsFalse(testObjects.Any(x => x is null));
        }

        [TestMethod]
        public void SingleThreadedList()
        {
            List<TestObject> testObjects = new();
            generatedObjects.ForEach(x => {
                testObjects.Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Count);
            Assert.IsFalse(testObjects.Any(x => x is null));
        }

        [ClassInitialize]
        public static void ClassInit(TestContext context)
        {
            for (int i = 0; i < 10000; i++)
            {
                generatedObjects.Add(MakeNewTestObject());
            }
        }

        public static int GetRandomPositiveIndex(int max)
        {
            var randomInteger = uint.MaxValue;
            while (randomInteger == uint.MaxValue)
            {
                byte[] data = RandomNumberGenerator.GetBytes(4);
                randomInteger = BitConverter.ToUInt32(data, 0);
            }

            return (int)(max * (randomInteger / (double)uint.MaxValue));
        }

        public static string GetRandomString(int characters) => new(Enumerable.Range(1, characters).Select(_ => chars[GetRandomPositiveIndex(chars.Length)]).ToArray());

        private const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
        static TestObject MakeNewTestObject(int fieldLength = 1000)
        {
            return new TestObject
            {
                FieldOne = GetRandomString(fieldLength),
                FieldTwo = GetRandomString(fieldLength),
                FieldThree = GetRandomString(fieldLength),
                FieldFour = GetRandomString(fieldLength),
            };
        }

        class TestObject
        {
            public string FieldOne { get; set; } = string.Empty;
            public string FieldTwo { get; set; } = string.Empty;
            public string FieldThree { get; set; } = string.Empty;
            public string FieldFour { get; set; } = string.Empty;
        }
    }
}

[Youssef1313]

@stephentoub Should this move to dotnet/runtime?

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Describe the problem you are trying to solve

A common pattern is to run parallel processing and to produce a set of data. The default collection types like List and HashSet are not threadsafe and you may get unexpected behavior (like nulls being added to the collection) if you modify them from multiple threads at the same time.

Describe suggestions on how to achieve the rule

I think there should be an analyzer that detects when you are using the non thread safe collections in the context of the built in parallelization mechanisms (ForAll or Parallel) and can swap out the collection you are using for a more appropriate collection.

Good Examples:

• Replace List with ConcurrentBag (add operations will not need to be changed, indexing operations would, which adds complexity to this)
• Replace Dictionary with ConcurrentDictionary

Other examples:

• Replace HashSet with ConcurrentDictionary<T,bool> (this is a workaround for there not being a threadsafe HashSet. The bool value is then ignored but you get threadsafety in a HashSet like structure.)

Additional context

The following set of tests demonstrates this issue in action along with potential rewrites.

namespace NonThreadSafeCollections
{
    using Microsoft.VisualStudio.TestPlatform.ObjectModel;
    using Microsoft.VisualStudio.TestTools.UnitTesting;
    using System;
    using System.Collections.Concurrent;
    using System.Collections.Generic;
    using System.Linq;
    using System.Security.Cryptography;

    [TestClass]
    public class NonThreadSafeCollections
    {
        static List<TestObject> generatedObjects = new List<TestObject>();
        
        [TestMethod]
        public void ParallelListNested()
        {
            Dictionary<(char, char), List<TestObject>> testObjects = new();
            generatedObjects.AsParallel().ForAll(x => {
                if (!testObjects.ContainsKey((x.FieldOne[0], x.FieldTwo[0])))
                {
                    testObjects[(x.FieldOne[0], x.FieldTwo[0])] = new List<TestObject>();
                }
                testObjects[(x.FieldOne[0], x.FieldTwo[0])].Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Values.Select(x => x.Count).Sum());
            Assert.IsFalse(testObjects.Any(x => x.Value.Any(y => y is null)));
        }

        [TestMethod]
        public void ParallelListNested_Fixed()
        {
            ConcurrentDictionary<(char, char), ConcurrentBag<TestObject>> testObjects = new();
            foreach(var character1 in chars)
            {
                foreach(var character2 in chars)
                {
                    testObjects[(character1, character2)] = new ConcurrentBag<TestObject>();
                }
            }
            generatedObjects.AsParallel().ForAll(x => {
                testObjects[(x.FieldOne[0], x.FieldTwo[0])].Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Values.Select(x => x.Count).Sum());
            Assert.IsFalse(testObjects.Any(x => x.Value.Any(y => y is null)));
        }

        [TestMethod]
        public void SingleThreadedListNested()
        {
            Dictionary<(char, char), List<TestObject>> testObjects = new();
            generatedObjects.ForEach(x => {
                if (!testObjects.ContainsKey((x.FieldOne[0], x.FieldTwo[0])))
                {
                    testObjects[(x.FieldOne[0], x.FieldTwo[0])] = new List<TestObject>();
                }
                testObjects[(x.FieldOne[0], x.FieldTwo[0])].Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Values.Select(x => x.Count).Sum());
            Assert.IsFalse(testObjects.Any(x => x.Value.Any(y => y is null)));
        }

        [TestMethod]
        public void ParallelList()
        {
            List<TestObject> testObjects = new();
            generatedObjects.AsParallel().ForAll(x => {
                testObjects.Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Count);
            Assert.IsFalse(testObjects.Any(x => x is null));
        }

        [TestMethod]
        public void ParallelList_Fixed()
        {
            ConcurrentBag<TestObject> testObjects = new();
            generatedObjects.AsParallel().ForAll(x => {
                testObjects.Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Count);
            Assert.IsFalse(testObjects.Any(x => x is null));
        }

        [TestMethod]
        public void SingleThreadedList()
        {
            List<TestObject> testObjects = new();
            generatedObjects.ForEach(x => {
                testObjects.Add(x);
            });
            Assert.AreEqual(generatedObjects.Count, testObjects.Count);
            Assert.IsFalse(testObjects.Any(x => x is null));
        }

        [ClassInitialize]
        public static void ClassInit(TestContext context)
        {
            for (int i = 0; i < 10000; i++)
            {
                generatedObjects.Add(MakeNewTestObject());
            }
        }

        public static int GetRandomPositiveIndex(int max)
        {
            var randomInteger = uint.MaxValue;
            while (randomInteger == uint.MaxValue)
            {
                byte[] data = RandomNumberGenerator.GetBytes(4);
                randomInteger = BitConverter.ToUInt32(data, 0);
            }

            return (int)(max * (randomInteger / (double)uint.MaxValue));
        }

        public static string GetRandomString(int characters) => new(Enumerable.Range(1, characters).Select(_ => chars[GetRandomPositiveIndex(chars.Length)]).ToArray());

        private const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
        static TestObject MakeNewTestObject(int fieldLength = 1000)
        {
            return new TestObject
            {
                FieldOne = GetRandomString(fieldLength),
                FieldTwo = GetRandomString(fieldLength),
                FieldThree = GetRandomString(fieldLength),
                FieldFour = GetRandomString(fieldLength),
            };
        }

        class TestObject
        {
            public string FieldOne { get; set; } = string.Empty;
            public string FieldTwo { get; set; } = string.Empty;
            public string FieldThree { get; set; } = string.Empty;
            public string FieldFour { get; set; } = string.Empty;
        }
    }
}

Author:	gfs
Assignees:	-
Labels:	api-suggestion, area-System.Collections, code-analyzer
Milestone:	-