.Net BlockingCollection not work with Parallel.foreach

Question

I have tried to feed a dictionary with the values of a BlockingCollection

(...)

lock (blockingCol)
{
    lock (cache)
    {
        cache = new Dictionary<Guid, customDocument>();

        Parallel.ForEach(blockingCol,
            (document) =>
            {
                lock (cache) //Same bug with or without this lock
                    cache.Add(document.Id, document);
            });

        blockingCol = new BlockingCollection<customDocument>();

        (...)
    }

     (...)
}

(...)

The problem is sometimes the document is already in the cache. But I don't understand why. I create a new cache before and the BlockingCollection should not dequeue the same document twice. But I think it's a problem of the Parallel because when I use the code under this it's work but I need a multithreading system here.

(...)

lock (blockingCol)
{
    lock (cache)
    {
        cache = new Dictionary<Guid, customDocument>();

        while (blockingCol.Count > 0)
        {
            var doc = _blockingCol.Take();
            cache.Add(doc.Id, doc);
        }

        blockingCol = new BlockingCollection<customDocument>();

        (...)
    }

    (...)
}

(...)

Admittedly, my multi-threading-fu is not strong, but I'm surprised this code does not deadlock being that you `lock(cache)` inside of a `lock(cache)`. Race condition, maybe? — Kenneth K., Jul 19 '17 at 15:06
Actually, on second look, I see that you assign to `cache` inside of the outer lock. Since `cache` now points to a new object, I suppose that's why there wouldn't be a deadlock. — Kenneth K., Jul 19 '17 at 15:07
If i understand corrrectly this : [Lock inside lock](https://stackoverflow.com/questions/9172934/lock-inside-lock). I can because it's the same thread. But it's not multithreading... I'm agree with you, here I can't lock in a lock. — CrunchyArtie, Jul 19 '17 at 15:11
Just a fyi. If you are using a blocking collection you need to call `.GetConsumingEnumerable()` and pass that enumerable in to your foreach blocks. — Scott Chamberlain, Jul 19 '17 at 15:18
Just a fyi. Afaik a `BlockingCollection` is already threadsafe and if you swap the `Dictionary` with a `ConcurrentDictionary` you do not need the locks anymore. — Peter Bons, Jul 19 '17 at 15:18

score 1 · Answer 1 · answered Jul 30 '17 at 19:33

The problem is neither with BlockingCollection nor with Parallel.ForEach() call nor with locking, though you can get rid of explicit locking with proper usage of concurrent data structures. It is with the fact that you are using a plain Dictionary<> inside multithreaded code.

Rewrite your snippet to be

    Parallel.ForEach(blockingCol.GetConsumingEnumerable(),
        (document) =>
        {
             cache.AddOrUpdate(document.Id, document, (k,v) => document);
        });

and it will get rid of the exception, ArgumentException with message 'An element with the same key already exists in the Dictionary.'

While this solves your immediate problem, you should rewrite the section completely to be simply

cache = blockingCol.ToDictionary( d => d.Id );

That Parallel.ForEach() does not buy you anything in terms of performance.

Also, check your producer side to make sure it does not put two documents with same Id.

.Net BlockingCollection not work with Parallel.foreach

1 Answers1