0

In my program, i'm iterating through a list of 'Group' objects using a Parallel.Foreach loop. Inside this loop, I first check my concurrentdictionary if a key exists, and if the value contains a Group property. I then add an object to a list depending on whether or not the dictionary has the key and value. Code shown below:

var roleUsers = new ExtendedBindingList<RoleUser>();
ConcurrentDictionary<int, List<int>> roleMatch = new ConcurrentDictionary<int, List<int>>();

Parallel.ForEach(groupsWithRole, group =>
            {
                foreach (var u in usersInThisGroup[group.GroupID])
                {
                    if (roleMatch.ContainsKey(u.UserID) && roleMatch[u.UserID].Contains(group.RoleID))
                        continue;

                    //
                    //Unimportant logic
                    //

                        lock (writelock)
                        {
                            roleUsers.Add(roleUser);
                            if (!roleMatch.ContainsKey(u.UserID))
                                roleMatch.TryAdd(u.UserID, new List<int>());
                            roleMatch[u.UserID].Add(group.RoleID);
                        }
                    }
                }
            });

I've come to find that the list roleUsers doesn't always have the same number of objects in it, when it very much should. It's quite obviously a threading issue. My question is, is there any way, besides locking the whole thing, to read and write to the concurrentdictionary safely?

Steve
  • 571
  • 8
  • 16
  • 3
    Aside from anything else, you're using `List` from multiple threads - *that's* not safe without a lock. You should consider using a thread-safe collection as the value of the dictionary. – Jon Skeet Apr 07 '15 at 16:42
  • 1
    As a secondary thought - have you considered partitioning your data by group ID, so that only a single thread handles each group? That could make things a lot simpler. – Jon Skeet Apr 07 '15 at 16:45
  • @JonSkeet There is already a lock around the list. I was originally using a ConcurrentBag but since i needed a lock around the dictionary adding, i ended up making it a list and having it locked as well. As for partitioning, currently each group in the foreach is unique. I may be able to partition it further, but i was looking for a more proper way to use the dictionary if possible. – Steve Apr 07 '15 at 16:55
  • 1
    There isn't a lock when you call `Contains` though, is there? So one thread could be calling `Contains` while another is adding to the list. And you were asking about trying to remove the lock - which would make it even less safe in terms of list access. – Jon Skeet Apr 07 '15 at 16:56
  • Sorry, I misread the code - I think I meant group by user ID. – Jon Skeet Apr 07 '15 at 16:57
  • Oh you are right, i thought you were looking at the roleUsers list inside the lock. My mistake. I will look into using a different structure for it. Edit: Replaced it with a concurrentbag, since order doesn't matter for me – Steve Apr 07 '15 at 16:59
  • @JonSkeet You were right to partition by UserID. I rewrote the loop to use UserID instead of GroupID. This allows me to remove the lock around writing and reading, and the resulting Bag returns correct amount of items each time. Cut execution time from 0.15seconds to 0.068seconds too. Thanks Jon! – Steve Apr 07 '15 at 18:49

0 Answers0