мопэд не мой, eсть такой нерусский код, Cache.Storage[Key] произвольно иногда раз в неделю иногда 2 раза в день (с нагрузкой не сильно связано) выдает данные для чужих "id". Вопросы:

1. Как с минимальным шумом пофиксить
2. Как подтвердить некорректность работы простым тестом

void Process(id)
{
   Cache.Populate(id);

   // периодически выдает данные для чужих "id" 
   var key = Cache.Storage[Key];
}

...

static class Cache
{
   private static readonly object _lock = new object();
   public static Hashtable { get; set; } Storage = new Hashtable();

   public static void Populate(int id)
   {
      lock(_lock )
      {
          Storage[Key] = GetFromDb(id);
      }
   }
}

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>1. Как с минимальным шумом пофиксить

void Process(id)
{
   var key = Cache.Populate(id);
}

...

static class Cache
{
   private static readonly object _lock = new object();
   public static Hashtable { get; set; } Storage = new Hashtable();

   public static object Populate(int id)
   {
      lock(_lock )
      {
          var data = GetFromDb(id);
          Storage[Key] = data;
          return data;
      }
   }
}

尿Ǥ푙>2. Как подтвердить некорректность работы простым тестом

Для этого нужно знать кто такой Key, что за GetFromDb и что закладывалось в эту наркоманию.
Если Key общий и в него пишут все подряд с разными id, то внезапно между вызовом Populate и обращением к Storage[Key] может влезть кто угодно (написанный lock от этого внезапно не защищает).
Нужен вообще этот Storage? А то и кэширования как-то не видно.

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙> мопэд не мой, eсть такой нерусский код, Cache.Storage[Key] произвольно иногда раз в неделю иногда 2 раза в день (с нагрузкой не сильно связано) выдает данные для чужих "id". Вопросы:

尿Ǥ푙>1. Как с минимальным шумом пофиксить

Сделать словарь более типизированным.

尿Ǥ푙>2. Как подтвердить некорректность работы простым тестом

Добавить 2 разных объекта с одинаковых хэш-кодом. Можно переопределить соотв. функции.

K>Для этого нужно знать кто такой Key, что за GetFromDb и что закладывалось в эту наркоманию
K>Если Key общий и в него пишут все подряд с разными id, то внезапно между вызовом Populate и обращением к Storage[Key] может влезть кто угодно (написанный lock от этого внезапно не защищает).
K>Нужен вообще этот Storage? А то и кэширования как-то не видно.

Ключ — просто строка для хэштейбл что то типа "CustomerInventory", GetFromDb простой фетч данных из базы. Кэширование было когда то, сейчас его нет — данные обновляутся при каждом вызове даже для уже существующих. простого способа переделать вызов на инстансный нет т.к. репозиторий статически инициируется внутри этого статического класса, короче, без масшатбного рефакторинга не пройдет.

на простых тестах все прокатывает отлично, инстасный метода вызывает залоченный статичекий, я не знаю про особенности статичных классов и локов внутри них — юзал их только для Extension методов.

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙> мопэд не мой, eсть такой нерусский код, Cache.Storage[Key] произвольно иногда раз в неделю иногда 2 раза в день (с нагрузкой не сильно связано) выдает данные для чужих "id". Вопросы:

наверно дело в связи Key и id

хештейбл хранит проcто результат без привязки к айди "id" — по сути последний результат запрошенный Process(id) вызовом.

HashTable не является thread safe, что проявит себя тем или иным образом (для Dictionary обычно exception) при конкурентном вызове метода Process в разных потоках.
А ещё конкурентный поток может подменять Cache.Storage (объект и контент коллекции).

Поправить вышеуказанную проблему можно например так.

void Process(id)
{
   lock(Cache.Storage) {
       Cache.Populate(id);
    
       // периодически выдает данные для чужих "id" 
       var key = Cache.Storage[Key];
   }
}

Но лучше так:

static class Cache
{
   public static Hashtable { get; } Storage = Hashtable.Synchronized(new Hashtable());

   public static void Populate(int id)
   {      
          Storage[Key] = GetFromDb(id);      
   }
}

Дополнено: Из кода не вполне ясно, что именно _lock синхронизировал. Возможно GetFromDb. Тогда _lock всё же нужен.

спасибо. будем пробовать. каким тестом можно подтвердить проблему?

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>Ключ — просто строка для хэштейбл что то типа "CustomerInventory", GetFromDb простой фетч данных из базы. Кэширование было когда то, сейчас его нет — данные обновляутся при каждом вызове даже для уже существующих. простого способа переделать вызов на инстансный нет т.к. репозиторий статически инициируется внутри этого статического класса, короче, без масшатбного рефакторинга не пройдет.

Так и выкинуть Storage, чтобы память лишнюю бесполезно не занимал.
Пусть Populate сразу данные возвращает без отдельных запросов и не будет проблем.
Для тестирования нужно несколько потоков создать, чтобы параллельно вызывали Process(id) для разных идентификаторов. Ошибка должна по идее достаточно часто проявляться (если между вызовом Populate и Storage[Key] воткнуть задержку, так и к 100% ошибок можно дойти).

尿Ǥ푙>спасибо. будем пробовать. каким тестом можно подтвердить проблему?

race condition простым тестом не подтвердить.

Если значение Key общее для разных id, то вероятная причина проблемы описана в первом комментарии (от karbofos42).

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙> Cache.Populate(id);
Защищено от конкурентного доступа локом.

尿Ǥ푙> var key = Cache.Storage[Key];
Не защищено от конкурентного доступа локом.
Классическая ошибка чтения незащищённых данных из многопоточки — хорошо работает _почти_ всегда.
Решение — выкинуть локи и использовать ConcurrentDictionary.

Попробуем, спасибо. Process(id) часто вызывается из Parallel.ForEach — так что очень похоже на причину и чем лочить это все, скорее всего проще переделать на ConcurrentDictionary.

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>Попробуем, спасибо. Process(id) часто вызывается из Parallel.ForEach — так что очень похоже на причину и чем лочить это все, скорее всего проще переделать на ConcurrentDictionary.

тут нужен либо lock на весь Process(id), либо выкидывать ненужную операцию записи в ненужный словарь и одним махом брать данные из БД.
Просто поменять Hashtable на ConcurrentDictionary и выкинуть lock — останется то же самое, что было.
ConcurrentDictionary отдельные операции записи и чтения никак волшебным образом не синхронизирует.
Так же сначала один поток вызовет Populate(1) и запишет данные в словарь.
Потом второй поток вызовет Populate(2) и перепишет данные в словаре на свои.
Потом первый поток проснётся и через Storage[Key] получит не данные для id == 1, а то, что записал второй поток для id == 2.

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>хештейбл хранит проcто результат без привязки к айди "id" — по сути последний результат запрошенный Process(id) вызовом.
тогда не понятно как вы определяете что выдан результат для чужих айди
т.е. проблема не в этом коде, а в том который кэш использует.

Здравствуйте, m2user, Вы писали:

M>HashTable не является thread safe, что проявит себя тем или иным образом (для Dictionary обычно exception) при конкурентном вызове метода Process в разных потоках.

В коде у ТС запись в hashtable защищена локом, а hashtable, в отличии от Dictionary, гарантирует N одновременных читателей при 1-м писателе, так что тут всё норм.

Здравствуйте, ·, Вы писали:

尿Ǥ푙>> var key = Cache.Storage[Key];
·>Не защищено от конкурентного доступа локом.
·>Классическая ошибка чтения незащищённых данных из многопоточки — хорошо работает _почти_ всегда.
·>Решение — выкинуть локи и использовать ConcurrentDictionary.

В текущей логике ConcurrentDictionary не поможет, см. тут

Автор: pilgrim_
Дата: 12.10.23

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙> мопэд не мой, eсть такой нерусский код, Cache.Storage[Key] произвольно иногда раз в неделю иногда 2 раза в день (с нагрузкой не сильно связано) выдает данные для чужих "id". Вопросы:

尿Ǥ푙>1. Как с минимальным шумом пофиксить
尿Ǥ푙>2. Как подтвердить некорректность работы простым тестом

1) Если код Process "линейный", т.е. не создаёт в процессе работы доп. потоков из которых есть обращение к Cache.Storage, то имхо самое простое это повесить на Cache.Storage атрибут AsyncLocal (или ThreadStatic для старого .NET, который судя по наличии Hashtable может быть).

2) Как уже подсказали, после вызова Populate поставить паузу, в процессе которой гарантированно прилетит вызов Process с другим id.

_>В коде у ТС запись в hashtable защищена локом, а hashtable, в отличии от Dictionary, гарантирует N одновременных читателей при 1-м писателе, так что тут всё норм.

Действительно

Hashtable is thread safe for use by multiple reader threads and a single writing thread.

Правда, остается возможность конкурентной записи через поле Storage.

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:



尿Ǥ푙>[code]
尿Ǥ푙>void Process(id)
尿Ǥ푙>{
尿Ǥ푙> Cache.Populate(id);

尿Ǥ푙> // периодически выдает данные для чужих "id"
尿Ǥ푙> var key = Cache.Storage[Key];
尿Ǥ푙>}
Зачем вам кэш если вы не можете идентифицировать данные?

void Process(id) { var key =  GetFromDb(id); }