CODEWITHSUNDEEP
c#multithreadinglocking
colinfang
colinfang

Reputation: 21727

How to improve this objects dictionary caching lock in multi-thread (i.e. singleton with parameters)?

public class JobModel
{
    static readonly Object _padlock = new object();
    static readonly Dictionary<Tuple<int, string>, JobModel> _jobModelCache = new Dictionary<Tuple<int, string>, JobModel> ();

    public static JobModel Get(int jobId, string model)
    {
        lock (_padlock) {
            JobModel jobModel;
            var key = Tuple.Create (jobId, model);
            var success = _jobModelCache.TryGetValue (key, out jobModel);
            Console.WriteLine (success);
            if (success)
                return jobModel;
            else {
                jobModel = new JobModel (jobId, model);
                _jobModelCache [key] = jobModel;
                return jobModel;
            }
        }
    }
...
}

The instance constructor of JobModel is unfortunately not thread-safe. However, I want to make JobModel.Get thread-safe and return the same instance given same parameters.

From Implementing the Singleton Pattern in C# I read about that it is better not to do double-check locking.

So my attempt is not very good since it locks on retrieval as well.

Can I get some advice on how to improve the code in my use case?

Update

Ok, I think I have found a workaround, from @svick in is-this-code-thread-safe-singleton-implementation-using-concurrent-dictionary

Never create two connection objects with the same parameters. If one exists, use it.

If you really need to guarantee this, then I think you will need to use locking instead of ConcurrentDictionary.

If it's okay to create duplicate Connections (that will never be used) in rare circumstances, then you can use an overload of GetOrAdd() that takes a lambda that creates the Connection:

return activeConnections.GetOrAdd(
    Tuple.Create(param1,param2), _ => new Connection (param1, param2));

So perhaps I can do

public class JobModel
{
    static readonly Object _padlock = new object();
    static readonly ConcurrentDictionary<Tuple<int, string>, JobModel> _jobModelCache = new ConcurrentDictionary<Tuple<int, string>, JobModel> ();

    public static JobModel Get(int jobId, string model)
    {
        // Still not perfect, since it might call the instance constructor multiple times.
        // Lock instance constructor as it is not thread-safe.
        return _jobModelCache.GetOrAdd (Tuple.Create (jobId, model), 
                                        _ => {
            lock (_padlock) {
                let ret = new JobModel (jobId, model);
                return ret;
            }
        }
        );
    }
    }
...
}

Upvotes: 0

Views: 631

Answers (1)

svick
svick

Reputation: 244837

I didn't choose ConcurrentDictionary since its value might not be consistent.

If you only use GetOrAdd(TKey, Func<TKey, TValue>) and don't call any other methods on the ConcurrentDictionary (like AddOrUpdate()), then the value will be consistent: for a given key, GetOrAdd() will always return the same value.

What is not guaranteed is that only one value will ever be created for the key. What can happen is that two threads call GetOrAdd() at the same time, which will result in creating two values. But one of them will be thrown away, and GetOrAdd() will return the same value on both threads.

If this is not okay for you and the performance of locking always on a normal Dictionary is acceptable, then use that.

If it's not acceptable, then you will need some smarter solution. But I don't think normal double-checked locking would work here, because it's not okay to read and write to a Dictionary at the same time, while it is okay to read and write a volatile reference, if you do it correctly. (Writing a volatile reference, as is done in the normal double-checked singleton, is atomic. Writing to a Dictionary is not.)

Upvotes: 1

Related Questions