Use LINQ ToDictionary with ConcurrentDictionary

Question

I'm using a dictionary to collect events in a multithread application, using lock when I add an event and not using it when I search for one. Every hour or so I run a cleanup of the events older than a certain time. Very simple and it works.

I'd like to move to a ConcurrentDictionary to remove the locks, and I thought I just had to add "Concurrent" and change the Add to TryAdd. But then I incurred in the error that LINQ returns only ToDictionary. I can obviusly not use LINQ, but I was curios to know if there's something I can do to preserve it. And more important, is there something I else I should consider before moving to ConcurrentDictionry?

public class messageResult
        {
            public Result result;
            public DateTime receivedTime;
        }


public Dictionary<Guid, messageResult> events = new Dictionary<Guid, messageResult>();


lock (events)
            {
                events = events.Where(p => p.Value.receivedTime >= t).ToDictionary(p => p.Key, p => p.Value);
            }

Thanks

"*using lock when I add an event and **not using it** when I search for one*" Locks don't work that way, you must lock when reading to prevent a write happening while you read. However you can do what you want to do (allow multiple readers and a single writer) by useing [ReaderWriterLockSlim](http://msdn.microsoft.com/en-us/library/system.threading.readerwriterlockslim.aspx) instead, this will let you have many readers who all stop when the one writer wants to write. — Scott Chamberlain, Oct 01 '13 at 21:25
@L.B thanks for the suggestion, but what are the advantages over my shared Dictionary instance? — Mattia Durli, Oct 04 '13 at 09:58
@ScottChamberlain I'm certain that no thread is going to read the value I'm writing for at least a few seconds, that's why I thought that locking only when writing was sufficient. Am I wrong? Should I really lock every time I access the object? I'll check anyway your suggestion for ReaderWriterLockSlim — Mattia Durli, Oct 04 '13 at 10:03
@MattiaDurli It's not that you need to lock on the same key, you need to lock on the same bucket. If you try to read and write to the same bucket inside the dictionary it can cause problems. — Scott Chamberlain, Oct 04 '13 at 13:37
@ScottChamberlain Ok, but can you make an example of what kind of problems? just to understand. Because I can understand problems when trying more writes at the same time, or read of a value being written, but what can go wrong if I try to read values that I'm sure have already been written at least a few second before? By the way, using a ConcurrentDictionary would solve the problem right? — Mattia Durli, Oct 05 '13 at 11:11

score 0 · Answer 1 · answered Oct 01 '13 at 20:59

0

It looks like the ToDictionary extension method is not so complicated and you can create your own version.

But note that ToDictionary returns new object while you would like to have one dictionary and share it between your threads.

You should not lock on mutable variable (and you are also changing the actual reference to events), create private readonly variable and use it for locking.

answered Oct 01 '13 at 20:59

Karel Frajták

4,389
1
23
34

Can you elaborate "create private readonly variable and use it for locking"? I don't understand how can I use a readonly variable in this context – Mattia Durli Oct 04 '13 at 10:11
http://stackoverflow.com/questions/5053172/why-does-the-lock-object-have-to-be-static – Karel Frajták Oct 04 '13 at 13:15

score 0 · Answer 2 · answered Oct 01 '13 at 21:14

0

ToDictionary returns a new instance... which is different than your shared instance.

You want to modify your shared instance and should write code which does that:

foreach(var kvp in events.Where(...).ToList())
{
  var val = kvp.Value;
  events.TryRemove(kvp.Key out val);
}

answered Oct 01 '13 at 21:14

Amy B

108,202
21
135
185

Yes you're right, it returns a new instance, but since it's in a lock it should work (actually it works). Is it wrong what I'm doing or just unperformant? – Mattia Durli Oct 04 '13 at 10:06
By the way, your suggestion is to use a ConcurrentDictionay, but then why to iterate the elements you convert it to a List, can't I iterate directly through the ConcurrentDictionary elements and TryRemove the expired ones? – Mattia Durli Oct 04 '13 at 10:08

Use LINQ ToDictionary with ConcurrentDictionary

2 Answers2