Threading with Parallel.For Adding Lists

Question

I am having trouble using the Parallel.For method. I am making a GET call to get back a list. Then I want to take that list and add it to the main list. I have tried addRange which is not thread safe, and will return the wrong data in the list. I have also tried to use ConcurrentBag which also does not get the right data. When I say does not get the right data I mean some of the data in the lists is either repeating or gets over written.

Here is my code(updated):

var thisLock = new Object();
var list = new List<Person>();
Parallel.For(1, 10, x =>
{
    request.Page = x;
    response = Get(request); // call to client
    lock(thisLock)
    {
        list.AddRange(response);
    }
}

Any other ideas besides addRange or ConcurrentBag

locking around list. There are other mechanisms but this one is simplest, as adding to the list is not likely the major performance bottleneck in the code. — Rotem
– Rotem, Commented Jun 11, 2014 at 14:08
As @Rotem intimated, you need to lock the list when adding to it. Concurrentbag should deal with that. What do you mean doesn't get the right data? — Tony Hopkinson
– Tony Hopkinson, Commented Jun 11, 2014 at 14:11
when I lock the main list will only mess up one call. Meaning the data in one part of the main list will be over written. — Beastwood
– Beastwood, Commented Jun 11, 2014 at 14:21
@Beastwood that's not the correct way to lock, you should define a specific locking object instead. Regardless, a ConcurrentBag<T> is the recommended collection for what you are trying to do. The fact that your data is "duplicated" suggests to me that your server is returning duplicate data rather than the code somehow duplicating it. — James
– James, Commented Jun 11, 2014 at 14:24

James · Accepted Answer · 2014-06-11 15:14:09Z

4

I am making a few assumptions here, but it would appear that your problem is the fact that your request/response variables are not scoped within the Parallel.For call.

The problem is you make a (presumably) synchronous Get call which updates the response variable but given you have X threads all working with that very same response if that gets updated at any given point i.e. whilst another thread is adding it to the list, then that is going to very well lead to duplicate data.

Same goes for the request, you have an obvious race condition meaning that when one thread changes the request.Page and another thread is just about to pull the data then you are effectively pulling the same page across various threads.

The solution is simple, create your request/response objects locally

var list = new ConcurrentBag<T>();
Parallel.For(1, 10, x =>
{
    var request = // create new request;
    request.Page = x;
    var response = Get(request); // call to client
    foreach (var item in response) {
        list.Add(item); // add it main list
    }
}

edited Jun 11, 2014 at 15:14

answered Jun 11, 2014 at 14:35

James

82.4k19 gold badges172 silver badges245 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Rotem Over a year ago

+1 This is most likely it, I can't believe I missed it.

Beastwood Over a year ago

There is no AddRange for ConcurrentBag. Should I just do a loop to add each one individually

Beastwood Over a year ago

@James Thanks for the help. I actually also had to lock with the ConcurrentBag for some reason to get it to work. And then it still only works about 75% of the time.

James Over a year ago

@Beastwood you definitely shouldn't need to lock if you are using ConcurrentBag<T>... what do you mean it only works 75% of the time? Are you re-creating both the request & response in each thread as suggested?

Beastwood Over a year ago

@James I think I might have fixed it. I about to run a lot of tests, but I think I made a dumb mistake. I declared a new request, but i had one declared above, and I just said var newResponse = resonse. And as you can tell I am not that good at programming(YET), but I could be wrong but by doing that I was still pointing at the same object in memory so now I made a brand new request

Douglas · Accepted Answer · 2014-06-11 20:13:20Z

0

This is a good candidate for PLINQ. You can then use SelectMany for flattening the sequences.

var list = ParallelEnumerable.Range(1, 10).SelectMany(x =>
{
    var request = // create new request;
    request.Page = x;
    response = Get(request); // call to client
    return response;
}).ToList();

answered Jun 11, 2014 at 20:13

Douglas

55.1k14 gold badges145 silver badges193 bronze badges

1 Comment

Beastwood Over a year ago

thanks for the alternative. I tried this way and it worked, but it was a little slower.

Collectives™ on Stack Overflow

Threading with Parallel.For Adding Lists

2 Answers 2

5 Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related