CODEWITHSUNDEEP
c#parallel-processingtask-parallel-libraryparallel.foreach
Martijn
Martijn

Reputation: 24789

Unexpected behavior when using Parallel.Foreach

I have this code:

// positions is a List<Position>
Parallel.ForEach(positions, (position) =>
{
    DeterminePostPieceIsVisited(position, postPieces);
});

private void DeterminePostPieceIsVisited(Position position, IEnumerable<Postpieces> postPieces)
{
    foreach (var postPiece in postPieces)
    {
        if (postPiece.Deliverd)
            continue;

            var distanceToClosestPosition = postPiece.GPS.Distance(position.GPS);

            postPiece.Deliverd = distanceToClosestPosition.HasValue && IsInRadius(distanceToClosestPosition.Value);
        }
    }
}

I know that 50 post pieces must have the property Deliverd set to true. But, when running this code, I get changing results. Sometimes I get 44, when I run it another time I get 47. The results are per execution different.

When I run this code using a plain foreach-loop I get the expected result. So I know my implementation of the method DeterminePostPieceIsVisited is correct.

Could someone explain to me why using the Parallel foreach gives me different results each time I execute this code?

Upvotes: 3

Views: 910

Answers (2)

Martijn
Martijn

Reputation: 24789

I have solved my issue with ConcurrentBag<T>. Here's what I use now:

var concurrentPostPiecesList = new ConcurrentBag<Postpiece>(postPieces);
Parallel.ForEach(positions, (position) => 
{
    DeterminePostPieceIsVisited(position, concurrentPostPiecesList);
});

private void DeterminePostPieceIsVisited(Position position, ConcurrentBag<Postpieces> postPieces)
{
    foreach (var postPiece in postPieces)
    {
        if (postPiece.Deliverd)
            continue;

        var distanceToClosestPosition = postPiece.GPS.Distance(position.GPS);

        postPiece.Deliverd = distanceToClosestPosition.HasValue && IsInRadius(distanceToClosestPosition.Value);
    }
}

Upvotes: 0

Damien_The_Unbeliever
Damien_The_Unbeliever

Reputation: 239664

You've already, I think, tried to avoid a race, but there is still one - if two threads are examining the same postPiece at the same time, they may both observe that Deliverd (sic) is false, and then both assess whether it's been delivered to position (a distinct value for each thread) and both attempt to set a value for Deliverd - and often, I would guess, one of them will be trying to set it to false. Simple fix:

private void DeterminePostPieceIsVisited(Position position, IEnumerable<Postpieces> postPieces)
{
    foreach (var postPiece in postPieces)
    {
        if (postPiece.Deliverd)
            continue;

        var distanceToClosestPosition = postPiece.GPS.Distance(position.GPS);

        var delivered = distanceToClosestPosition.HasValue && IsInRadius(distanceToClosestPosition.Value);
        if(delivered)
            postPiece.Deliverd = true;
    }
}

Also, by the way:

When I run this code using a plain foreach-loop I get the expected result. So I know my implementation of the method DeterminePostPieceIsVisited is correct.

The correct thing to state is would be "I know my implementation is correct for single threaded access" - what you hadn't established is that the method was safe for calling from multiple threads.

Upvotes: 3

Related Questions