Is this Method Thread-Safe?

Question

Can someone please tell me if the following method is thread safe. Also, please assume the call to _cache.GetOrCreate(...) is thread-safe. This method is the ONLY place in the application that creates or updates the region (dictionary). The class containing this method is a singleton, so multiple threads will access it.

    public IEnumerable<string> GetReportLookupItems<T>(string cacheKey, Func<IEnumerable<string>> factory)
    {
        Dictionary<string, IEnumerable<string>> region = _cache.GetOrCreate("Cache-Region:LookupItems", () => new Dictionary<string, IEnumerable<string>>());

        IEnumerable<string> items;

        if (!region.TryGetValue(cacheKey, out items))
        {
            region[cacheKey] = items = factory();
        }

        return items;
    }

Even if you did a double-checked pattern, it wouldn't be thread safe: http://stackoverflow.com/questions/2624301/how-to-show-that-the-double-checked-lock-pattern-with-dictionarys-trygetvalue-is — Amir, Jan 25 '11 at 21:15
You might find http://msdn.microsoft.com/en-us/library/system.threading.readerwriterlock.aspx useful, especially since it includes an example of how to make access to a dictionary thread safe. — Juliet, Jan 25 '11 at 22:22

score 5 · Accepted Answer · answered Jan 25 '11 at 20:30

5

No. It's definitely not thread safe.

You're using a Dictionary<T,U> here, and changing its contents. Since Dictionary<T,U> is not thread-safe, the calls to TryGetValue and setting the dictionary by key are not thread safe.

If you were to change this to use a thread-safe dictionary (such as ConcurrentDictionary<T,U>), you could potentially have this be a thread-safe method.

answered Jan 25 '11 at 20:30

Reed Copsey

554,122
78
1,158
1,373

1

Or if you cannot use `ConcurrentDictionary`, then you need to use lock() around the `if` block. – Erv Walter Jan 25 '11 at 20:35
3

But notice that even with a `ConcurrentDictionary` it’s not thread safe to invoke first `TryGetValue` and then subsequently add the value. Instead, `GetOrAdd` needs to be used. – Konrad Rudolph Jan 25 '11 at 20:36

score 1 · Answer 2 · answered Jan 25 '11 at 20:29

1

I would say it depends on what _cache.GetOrCreate doess.

But from the sound of it, it does not sound thread safe since it is accessing the same Dictionary.

answered Jan 25 '11 at 20:29

Daniel A. White

187,200
47
362
445

Like I mentioned, assume that method is thread-safe (which it is). – Marco Jan 25 '11 at 20:30

score 1 · Answer 3 · answered Jan 25 '11 at 20:32

1

No, this code is not thread safe because calling the instance method TryGetValue on a shared dictionary (returned by _cache.GetOrCreate) is not thread safe. Multiple threads could potentially invoke this instance method on the same dictionary instance at the same time.

answered Jan 25 '11 at 20:32

Darin Dimitrov

1,023,142
271
3,287
2,928

That’s not the problem. `TryGetValue` is a read-only operation. The problem is that the method subsequently *writes* to the dictionary. – Konrad Rudolph Jan 25 '11 at 20:38
@Konrad, that's true, but is there a guarantee that only reading from a dictionary would be thread-safe in all cases? – Darin Dimitrov Jan 25 '11 at 20:40
@Darin: the MSDN doesn’t mention the thread safety of this method – but we can still be reasonably certain with a bit of knowledge about hash tables that `TryGetValue` will be thread safe, since there’s no conceivable way that this method will change the object’s state. It’s strictly read-only. – Konrad Rudolph Jan 25 '11 at 20:48
@Konrad, `TryGetValue` enumerates and IMHO enumerating is not guaranteed to be thread-safe. Maybe I am mistaken here but would like to see a proof and MSDN clearly doesn't provide it. – Darin Dimitrov Jan 25 '11 at 20:52
“TryGetValue enumerates” – how do you make that out? It does no such thing (if you mean by “enumerates” that it creates an enumerator by calling `GetEnumerator`). Dictionaries are implemented via double hashing, not collision lists. But even if they were, the `GetEnumerator` method of `List` *is* thread-safe for read access. In particular, the documentation says “An enumerator remains valid as long as the collection remains unchanged” and goes on to imply that only writing access to a list ever needs synchronization. – Konrad Rudolph Jan 25 '11 at 21:02
That said, you certainly shouldn’t rely on this behaviour in principle so you are right there. However, I can state with a modicum of confidence that this is *not* the issue here. – Konrad Rudolph Jan 25 '11 at 21:03
@Konrad, totally agree with you, the OPs issue is that he is also writing to the dictionary, there are no two opinions here :-) I was just curious about the thread safety of the `TryGetValue` method assuming only concurrent reads and you make a good point. – Darin Dimitrov Jan 25 '11 at 21:05

score 1 · Answer 4 · answered Jan 25 '11 at 20:35

It is not thread safe.

Thread #1 executes all the way up to region[cacheKey] = items = factory();

Thread #1 is preempted by Thread #2

Thread #2 executes all the way up to region[cacheKey] = items = factory();

Thread #2 is preempted by Thread #1

Thread #1 executes factory()

Thread #1 is preempted by Thread #2

Thread #2 executes factory()

At this point in time you have two threads with two different instances of "factory()".

Is this Method Thread-Safe?

4 Answers4