Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ, Вы писали:
尿Ǥ푙>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? А то и кэширования как-то не видно.
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ, Вы писали:
尿Ǥ푙>Попробуем, спасибо. Process(id) часто вызывается из Parallel.ForEach — так что очень похоже на причину и чем лочить это все, скорее всего проще переделать на ConcurrentDictionary.
тут нужен либо lock на весь Process(id), либо выкидывать ненужную операцию записи в ненужный словарь и одним махом брать данные из БД.
Просто поменять Hashtable на ConcurrentDictionary и выкинуть lock — останется то же самое, что было.
ConcurrentDictionary отдельные операции записи и чтения никак волшебным образом не синхронизирует.
Так же сначала один поток вызовет Populate(1) и запишет данные в словарь.
Потом второй поток вызовет Populate(2) и перепишет данные в словаре на свои.
Потом первый поток проснётся и через Storage[Key] получит не данные для id == 1, а то, что записал второй поток для id == 2.
Здравствуйте, m2user, Вы писали:
M>HashTable не является thread safe, что проявит себя тем или иным образом (для Dictionary обычно exception) при конкурентном вызове метода Process в разных потоках.
В коде у ТС запись в hashtable защищена локом, а hashtable, в отличии от Dictionary, гарантирует N одновременных читателей при 1-м писателе, так что тут всё норм.
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 всё же нужен.
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ, Вы писали:
尿Ǥ푙> Cache.Populate(id);
Защищено от конкурентного доступа локом.
尿Ǥ푙> var key = Cache.Storage[Key];
Не защищено от конкурентного доступа локом.
Классическая ошибка чтения незащищённых данных из многопоточки — хорошо работает _почти_ всегда.
Решение — выкинуть локи и использовать ConcurrentDictionary.
но это не зря, хотя, может быть, невзначай
гÅрмония мира не знает границ — сейчас мы будем пить чай
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ, Вы писали:
尿Ǥ푙>хештейбл хранит проcто результат без привязки к айди "id" — по сути последний результат запрошенный Process(id) вызовом.
тогда не понятно как вы определяете что выдан результат для чужих айди
т.е. проблема не в этом коде, а в том который кэш использует.
尿Ǥ푙>[code] 尿Ǥ푙>void Process(id) 尿Ǥ푙>{ 尿Ǥ푙> Cache.Populate(id);
尿Ǥ푙> // периодически выдает данные для чужих "id" 尿Ǥ푙> var key = Cache.Storage[Key]; 尿Ǥ푙>}
Зачем вам кэш если вы не можете идентифицировать данные?
Здравствуйте, samius, Вы писали:
_>>В коде у ТС запись в hashtable защищена локом, а hashtable, в отличии от Dictionary, гарантирует N одновременных читателей при 1-м писателе, так что тут всё норм.
S>Все ли действительно норм — зависит не от того, помечен ли класс относительно потокобезопасным, а от того, совпадает ли ожидание пользователя кэша от того, как ведет себя коллекция.
"норм" относится к техническим деталям поведения hashtable.
S>А вести она себя будет так, что конкурирующие читатель и писатель не сломаются. Но что именно прочитает читатель, проскочив одновременно с писателем — точно не ясно. Либо старое значение, либо новое. Что из этого является норм, или сама неопределенность считается норм — вот тут уже надо спрашивать разработчика этого конкретного кэша. Судя, по тому, что написано в коде, ожидается, что читатели могут довольствоваться старыми значениями уже в тот момент, когда из базы прилетело новое и даже в тот момент, когда в хэштаблицу его уже "начали класть". Осью, вокруг которой крутится логика, является запись в volatile поле version. Но даже после его изменения читатель может все еще достать старое значение.
Всё верно, тот кто юзает hashtable во многопотоке должен понимать и принимать решение, как её возможности с пользой заюзать. К вопросу ТС это не относится, там что-то странное.
мопэд не мой, 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);
}
}
}
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ, Вы писали:
尿Ǥ푙> мопэд не мой, 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". Вопросы:
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ, Вы писали:
尿Ǥ푙>Ключ — просто строка для хэштейбл что то типа "CustomerInventory", GetFromDb простой фетч данных из базы. Кэширование было когда то, сейчас его нет — данные обновляутся при каждом вызове даже для уже существующих. простого способа переделать вызов на инстансный нет т.к. репозиторий статически инициируется внутри этого статического класса, короче, без масшатбного рефакторинга не пройдет.
Так и выкинуть Storage, чтобы память лишнюю бесполезно не занимал.
Пусть Populate сразу данные возвращает без отдельных запросов и не будет проблем.
Для тестирования нужно несколько потоков создать, чтобы параллельно вызывали Process(id) для разных идентификаторов. Ошибка должна по идее достаточно часто проявляться (если между вызовом Populate и Storage[Key] воткнуть задержку, так и к 100% ошибок можно дойти).
Попробуем, спасибо. Process(id) часто вызывается из Parallel.ForEach — так что очень похоже на причину и чем лочить это все, скорее всего проще переделать на ConcurrentDictionary.
Здравствуйте, ·, Вы писали:
尿Ǥ푙>> var key = Cache.Storage[Key]; ·>Не защищено от конкурентного доступа локом. ·>Классическая ошибка чтения незащищённых данных из многопоточки — хорошо работает _почти_ всегда. ·>Решение — выкинуть локи и использовать ConcurrentDictionary.
В текущей логике ConcurrentDictionary не поможет, см. тут
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ, Вы писали:
尿Ǥ푙> мопэд не мой, 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-м писателе, так что тут всё норм.
Здравствуйте, pilgrim_, Вы писали:
_>Здравствуйте, m2user, Вы писали:
M>>HashTable не является thread safe, что проявит себя тем или иным образом (для Dictionary обычно exception) при конкурентном вызове метода Process в разных потоках.
_>В коде у ТС запись в hashtable защищена локом, а hashtable, в отличии от Dictionary, гарантирует N одновременных читателей при 1-м писателе, так что тут всё норм.
Все ли действительно норм — зависит не от того, помечен ли класс относительно потокобезопасным, а от того, совпадает ли ожидание пользователя кэша от того, как ведет себя коллекция.
А вести она себя будет так, что конкурирующие читатель и писатель не сломаются. Но что именно прочитает читатель, проскочив одновременно с писателем — точно не ясно. Либо старое значение, либо новое. Что из этого является норм, или сама неопределенность считается норм — вот тут уже надо спрашивать разработчика этого конкретного кэша. Судя, по тому, что написано в коде, ожидается, что читатели могут довольствоваться старыми значениями уже в тот момент, когда из базы прилетело новое и даже в тот момент, когда в хэштаблицу его уже "начали класть". Осью, вокруг которой крутится логика, является запись в volatile поле version. Но даже после его изменения читатель может все еще достать старое значение.
сверху вниз — Таймер службы, потом Parallel.ForEach — то есть перекрывающие вызовы более чем возможны, если предидущий в середине своего Parallel.ForEach а новый уже перезапросил — Bob's your uncle.