Почему после проверки TryGetValue случается ошибка добавления элемента с таким же ключом?

Question

[Дмитрий Афонченко]

Добрый день, товарищи, имеется следующий код:

private class ThreadsafeMemoizeCache<TArgument, TResult>
        {
            private Dictionary<TArgument, TResult> cache = new Dictionary<TArgument, TResult>();
            
            public TResult GetOrAdd(TArgument key, Func<TArgument, TResult> valueFactory)
            {
                TResult result;
                
                if (cache.TryGetValue(key, out result))
                {
                    return result;
                }
                lock (this)
                {
                    if (!cache.TryGetValue(key, out result))
                    {
                        result = valueFactory(key);
                        Dictionary<TArgument, TResult> newCache = new Dictionary<TArgument, TResult>(cache);
                        newCache.Add(key, result);
                        cache = newCache;
                    }
                }
                return result;
            }
        }

Вопрос следующего рода. По какой-то причине в данном коде вылезает ошибка добавления уже существующего ключа. При этом такой же ключ добавляется этим же потоком. Попытка модифицировать код вот таким образом тоже ничего не дала:

lock (this)
                {
                    if (cache.TryGetValue(key, out result))
                    {
                        return result;
                    }

                    if (!cache.TryGetValue(key, out result))
                    {
                        result = valueFactory(key);
                        Dictionary<TArgument, TResult> newCache = new Dictionary<TArgument, TResult>(cache);
                        newCache.Add(key, result);
                        cache = newCache;
                    }
                }

В чем может быть причина?

Comments

[Larry Underwood]

Покажите вызывающий код, на первый взгляд проблем не вижу, в VS код тоже работает нормально при вызове GetOrAdd два раза с одним и тем же ключом.
PS: lock(this) - плохо, не юзайте его., юзайте lock(имя_приватного_поля_класса)

[Larry Underwood]

И еще момент, зачем создавать новый словарь newCache из старого cache каждый раз при отсутствии элемента?

[Дмитрий Афонченко]

Максим Ванюшкин: Чтобы гарантировать, что мы будем проводить манипуляции не со ссылкой на объект, который может изменяться, а со снимком этого необходимого объекта, который гарантированно меняться не будет. По крайней мере идея такая.)

[Larry Underwood]

Дмитрий Афонченко: дак ссылка на объект и не будет меняться, это гарантировано)) он же приватный

[Дмитрий Афонченко]

Максим Ванюшкин: Я могу, конечно показать вызывающий код, но думаю, от этого не станет понятнее.) Я просто рассчитывал, что найдется кто-то у кого случалась такая же ошибка и он знает в чем причина. На самом деле там еще в конструкторе этого Dictionary вылезает ошибка добавления ключа. Вот это вообще странно.

[Дмитрий Афонченко]

Максим Ванюшкин: "дак ссылка на объект и не будет меняться, это гарантировано)) он же приватный"
Я тоже так думаю, но автор этого кода решил иначе и наверняка не без оснований.) В любом случае, не хочу в это сильно вдумываться, пока не решу текущую проблему.)

[Сергей]

А зачем велосипеды? Есть же ConcurrentDictionary в System.Collections.Concurrent.

[Дмитрий Афонченко]

Сергей: Вот честно, в душе не знаю.) Код не мой, но возможно, что человек, который его писал делал его еще до появления ConcurrentDictionary.

[Сергей]

Дмитрий Афонченко: Ну тогда можно убрать локи и использовать его ;)

[Дмитрий Афонченко]

Сергей: Так и поступил в итоге.)

Answer 1

[Larry Underwood]

Во-первых, код я бы переписал так:

private class ThreadsafeMemoizeCache<TArgument, TResult>
    {
      private readonly Dictionary<TArgument, TResult> _internalCache = new Dictionary<TArgument, TResult>();

      private readonly object _internalCacheLocker = new object();

      public TResult GetOrAdd(TArgument key, Func<TArgument, TResult> valueFactory)
      {
        lock (_internalCacheLocker)
        {
          TResult result;
          if (_internalCache.TryGetValue(key, out result))
            return result;

          result = valueFactory(key);
          _internalCache.Add(key, result);
          return result;;
        }
      }
    }

А во-вторых, у меня вот такой код работает:

var cache = new ThreadsafeMemoizeCache<string, string>();
cache.GetOrAdd("1", (arg) => string.Empty);
cache.GetOrAdd("1", (arg) => string.Empty);

Comments

[Дмитрий Афонченко]

Убрал _internalCacheLocker и все заработало. Хотя ситуация очень странная. Ошибка была даже не в методе Add а еще в конструкторе, то есть если мы внутри внутреннего кэша создаем словарь на основе уже сущетсвующего, то в его глубинах происходит ошибка добавления уже существующего ключа. Причем она не всегда проявлялась, а только в некоторых случаях, которые, благо, легко воспроизводились.