.Net dictionary doesn't fetch value directly after inserting on rare occasions

Question

I have written following code snippet:

public interface IModelToViewModelServiceBase<in TDomain, out TViewModel>
    where TDomain : class, IDataModel
    where TViewModel : class, IDataModelViewModel
{
    TViewModel GetViewModel(TDomain model);
}

public abstract class ModelToViewModelServiceBase<TDomain, TViewModel> : IModelToViewModelServiceBase<TDomain, TViewModel>
    where TDomain : class, IDataModel
    where TViewModel : class, IDataModelViewModel
{
    private readonly IDictionary<TDomain, TViewModel> _modelToViewModel
        = new Dictionary<TDomain, TViewModel>();

    protected abstract TViewModel Create(TDomain model);

    public TViewModel GetViewModel(TDomain model)
    {
        if (model == null) return null;
        if (!_modelToViewModel.ContainsKey(model))
            _modelToViewModel[model] = Create(model);

        return _modelToViewModel[model];
    }
}

The purpose of this class doesn't matter for the problem. On rare occasions I get a KeyNotFound on the return-line. However, to my understanding the preceding if-clauses should prevent this from happening. No key can possibly be null and the retrieved value if not existing before was added in the previous instruction.

What am I missing here?

I now developed a workaround:

public interface IModelToViewModelServiceBase<in TDomain, out TViewModel>
    where TDomain : class, IDataModel
    where TViewModel : class, IDataModelViewModel
{
    TViewModel GetViewModel(TDomain model);
}

public abstract class ModelToViewModelServiceBase<TDomain, TViewModel> : IModelToViewModelServiceBase<TDomain, TViewModel>
    where TDomain : class, IDataModel
    where TViewModel : class, IDataModelViewModel
{
    private readonly IDictionary<TDomain, TViewModel> _modelToViewModel
        = new Dictionary<TDomain, TViewModel>();

    protected abstract TViewModel Create(TDomain model);

    public TViewModel GetViewModel(TDomain model)
    {
        if (model == null) return null;
        TViewModel viewModel = null;
        if (!_modelToViewModel.ContainsKey(model))
        {
            viewModel = Create(model);
            _modelToViewModel[model] = viewModel;
        }
        else
            viewModel = _modelToViewModel[model];

        return viewModel;
    }
}

This seems to work. However, this workaround shouldn't be necessary. May be this workaround is even better, because now one less access to the dictionary is executed. Still, the previous version should have worked always.

Update after answer:

@evk and @mjwills are both right. I didn't consider that my code is unsafe for concurrent use and multiple threads are accessing it. Hence, afer the suggestions of @mjwills the code looks as follows:

public interface IModelToViewModelServiceBase<in TDomain, out TViewModel>
    where TDomain : class, IDataModel
    where TViewModel : class, IDataModelViewModel
{
    TViewModel GetViewModel(TDomain model);
}

public abstract class ModelToViewModelServiceBase<TDomain, TViewModel> : IModelToViewModelServiceBase<TDomain, TViewModel>
    where TDomain : class, IDataModel
    where TViewModel : class, IDataModelViewModel
{
    private readonly ConcurrentDictionary<TDomain, TViewModel> _modelToViewModel
        = new ConcurrentDictionary<TDomain, TViewModel>();

    protected abstract TViewModel Create(TDomain model);

    public TViewModel GetViewModel(TDomain model)
    {
        if (model == null) return null;

        return _modelToViewModel.GetOrAdd(model, Create); ;
    }
}

Is `Equals()` and / or `GetHashCode()` overriden in `TDomain`? If so make sure `GetHashCode()` consistently returns the same value fo the same object and `Equals()` consistently returns `true` or `false` if objects are the same or not. — heijp06, Dec 25 '17 at 09:50
@mjwills Thank you for the advice to use the `TryGetValue` function. `GetViewModel` can be called from more than one thread. However, the first version still should prevent from `KeyNotFoundException`, because the Dictionary is private and there are no `Remove` calls to it yet. Hence, how should another thread remove the item in between the if-clause and the return? — Dima, Dec 25 '17 at 11:36
`GetViewModel can be called from more than one thread` Alas `Dictionary` is not thread-safe, so you can't call it from multiple threads without using `lock`. I would **strongly** suggest changing your code to use `ConcurrentDictionary` and then using https://msdn.microsoft.com/en-us/library/ee378676(v=vs.110).aspx . — mjwills, Dec 25 '17 at 11:44
@mjwills I wouldn't like to post a `TDomain` class which exhibits the issue. Considering the dictionary there isn't happening anything special. These classes even don't know anything about `ModelToViewModelServiceBase` or any of it's derived classes. And I looked it up again to make sure: They don't implement `Equals` or `GetHashCode`. — Dima, Dec 25 '17 at 11:47
Without an explicit `GetHashCode` implementation, a mutable object's hashcode could change - which _might_ explain what you are seeing (although the multi-threaded use of `Dictionary` is the more likely explanation). — mjwills, Dec 25 '17 at 11:49

Evk · Accepted Answer · 2017-12-25T11:58:54.633

You mentioned that your code can be accessed from multiple threads, but your code is not thread-safe. Is is not safe to write and read to regular Dictionary from multiple threads (while if you only read and never write - then it's ok). You might think that if you never remove items from dictionary then KeyNotFoundException might not be thrown in your case but that is not true. When you use structure in a way it was not designed to - anything can happen. For example consider this code:

class Program
{
    public static void Main(string[] args)
    {
        var service = new ModelToViewModelServiceBase();
        new Thread(() => AddServices(service)).Start();
        new Thread(() => AddServices(service)).Start();
        new Thread(() => AddServices(service)).Start();
        new Thread(() => AddServices(service)).Start();
        Console.ReadKey();
    }

    private static void AddServices(ModelToViewModelServiceBase services) {
        for (int i = 0; i < 100000; i++) {
            services.GetViewModel(i);
        }
    }
}

public class ModelToViewModelServiceBase {
    private readonly IDictionary<int, string> _modelToViewModel
        = new Dictionary<int, string>();

    protected string Create(int model) {
        return model.ToString();
    }

    public string GetViewModel(int model) {
        if (!_modelToViewModel.ContainsKey(model))
            _modelToViewModel[model] = Create(model);

        return _modelToViewModel[model];
    }
}

When you run it - you will almost always get KeyNotFoundException, while you never remove items from dictionary. That's because of how Dictionary implemented internally, and I think exact details are not relevant to this question.

Long story short - just don't use non-thread-safe structure from multiple threads (except when all threads are only reading and never writing) without proper synchronization, even if it might feel to you that it will work fine.

Thank you. You and @mjwills are right and @mjwills's suggestion to use the `ConcurrentDictionary` would prevent the `KeyNotFoundException` for both of our code snippets. — Dima, Dec 25 '17 at 12:14
@Dima note that if you will use `GetOrAdd` of `ConcurrentDictionary` and your view model implements `IDisposable` or just is heavy to create - you might fall into another trap, so consider reading this: https://andrewlock.net/making-getoradd-on-concurrentdictionary-thread-safe-using-lazy/ — Evk, Dec 25 '17 at 12:17
thank you, very much. I've read the article. So if the factory method returns an `IDisposable`, then the disposable which doesn't make it through to the dictionary, would need to be disposed. Hence, the use of the `Lazy` delegation. Okay, in my case this helps alot. — Dima, Dec 25 '17 at 17:33
Yep, `Lazy` would solve the 'created multiple times' issue @Dima . I personally avoid `Lazy` due to its default behaviour of exception caching. I would generally use `LazyWithNoExceptionCaching` instead - https://stackoverflow.com/a/42567351/34092 . _But it depends on whether `Create` can throw an exception or not._ — mjwills, Dec 25 '17 at 23:14

.Net dictionary doesn't fetch value directly after inserting on rare occasions

1 Answers1