Принципиально, по архитектуре — необходимо разделение двух типов объектов: охраняемого ресурса и блокировок.
То, что члены класса объявлены как статические, — нехорошо (даже если оставить в стороне вопиющие проблемы с инициализацией и разрушением объектов ядра). Шаблоны — это, конечно, круто. Но не в этом случае. Принципиальная проблема с шаблонами заключается в том, что они создаются во время компиляции, а набор блокируемых ресурсов во время компиляции может быть и неизвестен. Кроме того, даже если набор таких ресурсов в конкретном приложении статичен, подход с "волшебным" числом неверен по большому счету хотя бы потому, что в крупном проекте приведет к путанице, какие "волшебства" уже назначены, а какие нет.
Далее, счетчик читателей должен быть объявлен как volatile, т. к. все функции довольно просты, легко встраиваются и у компилятора может возникнуть страстное желание соптимизировать эту переменную.
Никакой нужды использовать Interocked-функции для счетчика чтецов.
Функции разблокировки удобнее просто разделить на UnlockReading и UnlockWriting, не требуя хранения флага m_write.
Здравствуйте , Вы писали:
>Функции разблокировки удобнее просто разделить на UnlockReading и UnlockWriting, не требуя хранения флага m_write.
Или использовать m_dwReaders (сделав int-ом) со отрицательным значением для писателей.
А>между 1 и 2 может случиться переключение контекста, тогда будет жопа
Объясни, не понял.
А>В деструкторе я бы сначала проверил m_write и только потом вызывал бы А>
А>WaitForSingleObject(mutex, INFINITE);
А>
Во время ухода последнего читателя вход должен быть заблокирован. Этот случай тоже проверять? Проще сделать как у меня.
А>Нет вызовов ClosaHandle().
Ну что делать.. При закрытии процесса объекты уйдут сами.
А>Короче представляю вторую версию класса.
А>class Locker2 А>{ А>public: А> Locker2() А> { А> mutex = CreateMutex(NULL, FALSE, NULL); А> event = CreateEvent(NULL, TRUE, TRUE, NULL); А> };
Это у тебя каждый раз статические объекты переинициализируются? Круто, круто...Смело. Или ты думаешь, что просто увеличиваешь счётчик ссылок объекта? Так ведь нет.. Хотя, можно использовать именованные объекты, тогда будет да.. Но значение описателя всё равно будет не такое, т.к. понятие "счётчик ссылок описателя" не существует...
А> void Want2Write() А> { А> HANDLE h[] = {event, mutex}; А> WaitForMultipleObjects(2, h, TRUE, INFINITE); А> m_write = true; А> };
После этого, насколько я понимаю, можно писать, да?
А> void Want2Read() А> { А> WaitForSingleObject(mutex, INFINITE); А> А> m_write = false;
А> if (!readers) А> SetEvent(event);
А> ReleaseMutex(mutex); А> InterlockedDecrement(&readers); А> };
А после этого, судя по названию читать.. Но почему readers уменьшается? И почему событие
устанавливается, оно должно сбрасываться
А> void Done() А> { А> if (!m_write) А> { А> WaitForSingleObject(mutex, INFINITE); А> if (!InterlockedDecrement(&readers)) //1 А> SetEvent(event); //2 А> ReleaseMutex(mutex); А> } А> else А> ReleaseMutex(mutex); А> };
А если переключение произойдёт между 1 и 2?
А> ~Locker2() А> { А> CloseHandle(mutex); А> CloseHandle(event); А> };
А теперь ты закрыл объекты, созданные в конструкторе... Очень смело.
А> LONG GetReaders(){return readers;};
А>private: А> static HANDLE mutex; А> static HANDLE event; А> static LONG readers; А> bool m_write; А>};
А>HANDLE Locker2::mutex = NULL; А>HANDLE Locker2::event = NULL; А>LONG Locker2::readers = 0;
Здравствуйте Аноним, Вы писали:
А>Теперь критика других вариантов.
А>Принципиально, по архитектуре — необходимо разделение двух типов объектов: охраняемого ресурса и блокировок.
Ты имеешь ввиду, разделение на CWriteGuard и CReadGuard? Мне не очевидна необходимсть такого разделения.
А>То, что члены класса объявлены как статические, — нехорошо (даже если оставить в стороне вопиющие проблемы с инициализацией и разрушением объектов ядра).
А по мне хорошо. Нужно было просто ввести дополнительный уровень абстакции — сделать статический объект на подобии твоего, а в конструкторе вызывать LockRead\LockWrite..
А>Шаблоны — это, конечно, круто. Но не в этом случае. Принципиальная проблема с шаблонами заключается в том, что они создаются во время компиляции, а набор блокируемых ресурсов во время компиляции может быть и неизвестен.
Т.е ты динамически определяешь, что где блокировать, а где нет? Круто. Я представляю, как это сделать, но пока не могу придумать соответствующей задачи. Да, для такого сдучая мой вариант не подходит.
А>Кроме того, даже если набор таких ресурсов в конкретном приложении статичен, подход с "волшебным" числом неверен по большому счету хотя бы потому, что в крупном проекте приведет к путанице, какие "волшебства" уже назначены, а какие нет.
typedef не поможет? Меня устаривает..
А>Далее, счетчик читателей должен быть объявлен как volatile, т. к. все функции довольно просты, легко встраиваются и у компилятора может возникнуть страстное желание соптимизировать эту переменную.
Согласен, исправлюсь.
А>Никакой нужды использовать Interocked-функции для счетчика чтецов.
Согласен.
А>Функции разблокировки удобнее просто разделить на UnlockReading и UnlockWriting, не требуя хранения флага m_write.
Так это помнить надо. Я отказался от функций блокировки/разблокировки как раз потому, что с ними нужно не забыть разблокировать. Так написал в начале функции SuperLocker lock(false) и больше ни о чём не думаешь. Не нужно на всех путях выхода из функции (в том числе и на исключениях) ставить lock.Unlock или нечто подобное.
Здравствуйте SergH, Вы писали:
SH>Здравствуйте Алекс, Вы писали:
А>>Навеяно предыдущим топиком. Сначала еще раз покритикую код SergH А>>В конструкторе А>>
А>>между 1 и 2 может случиться переключение контекста, тогда будет жопа
SH>Объясни, не понял.
ну вот ты записал в переменную например 1. Случилось переключение контекста, другой поток получил управление и хочет читать. Он туда записал 0. После этого твой поток снова получил управление. И тут условие
if (!m_write)
а m_write содержит уже неверное значение.
хъ
SH>Это у тебя каждый раз статические объекты переинициализируются? Круто, круто...Смело. Или ты думаешь, что просто увеличиваешь счётчик ссылок объекта? Так ведь нет.. Хотя, можно использовать именованные объекты, тогда будет да.. Но значение описателя всё равно будет не такое, т.к. понятие "счётчик ссылок описателя" не существует...
Здравствуйте Алекс, Вы писали:
SH>>Объясни, не понял.
А>ну вот ты записал в переменную например 1. Случилось переключение контекста, другой поток получил управление и хочет читать. Он туда записал 0. После этого твой поток снова получил управление. И тут условие А>
А>if (!m_write)
А>
А>а m_write содержит уже неверное значение.
Она же локальная. Два потока, одновременно исполняющие конструктор одного объекта... Такое не часто приснится..
А>>> void Done() А>>> { А>>> if (!m_write) А>>> { А>>> WaitForSingleObject(mutex, INFINITE); А>>> if (!InterlockedDecrement(&readers)) //1 А>>> SetEvent(event); //2 А>>> ReleaseMutex(mutex); А>>> } А>>> else А>>> ReleaseMutex(mutex); А>>> };
SH>>А если переключение произойдёт между 1 и 2?
А>Ну и пусть. Мы же защищены мутексом. Другой поток эту часть кода не исполнит.
Здравствуйте Алекс, Вы писали:
А>>В деструкторе я бы сначала проверил m_write и только потом вызывал бы А>>
А>>WaitForSingleObject(mutex, INFINITE);
А>>
SH>Во время ухода последнего читателя вход должен быть заблокирован. Этот случай тоже проверять? Проще сделать как у меня.
Вообще-то ты прав, я тебя просто не правильно понял.
А>>Нет вызовов ClosaHandle().
SH>Ну что делать.. При закрытии процесса объекты уйдут сами.
Можно ввести дополнительный уровень абстакции.
Делай что должно, и будь что будет
Re[4]: SRC: Locker2
От:
Аноним
Дата:
20.09.02 06:09
Оценка:
Здравствуйте SergH, Вы писали:
А>>Принципиально, по архитектуре — необходимо разделение двух типов объектов: охраняемого ресурса и блокировок.
SH>Ты имеешь ввиду, разделение на CWriteGuard и CReadGuard? Мне не очевидна необходимсть такого разделения.
Читай внимательно: охраняемого ресурса (т. е. секции) — CWriteGuard — и блокировок — CWriteGuard::CReadLock и CWriteGuard::CWriteLock. См. также код. Там нет никакого CReadGuard.
Т. е.
class CMyFragileResource : public CWriteGuard
{
// ...public:
void AnEvilModificationMethod(...)
{
CWriteLock wl(*this);
// ...
}
int ASaintLearningMethod(...) const
{
CReadLock rl(*this);
// ...
}
int AnUnprotectedMethod(...) const
{
// ...
}
CMyFragileResource& operator=(const CMyFragileResource& other)
{
if (this == &other)
return *this;
CWriteLock wl(*this);
CReadLock rl(other);
// ...return *this;
}
};
// ...
CMyFragileResource a, b;
a.AnEvilModificationMethod();
int z = b.ASaintLearningMethod();
a = b;
{
CMyFragileResource::CReadLock rl(a);
int y = a.AnUnprotectedMethod();
}
Пример предназначен только для иллюстрации использования класса CWriteGuard. Класс CMyFragileResource не является бузепречным образчиком проектирования.
А>>То, что члены класса объявлены как статические, — нехорошо (даже если оставить в стороне вопиющие проблемы с инициализацией и разрушением объектов ядра).
SH>А по мне хорошо. Нужно было просто ввести дополнительный уровень абстакции — сделать статический объект на подобии твоего, а в конструкторе вызывать LockRead\LockWrite..
Чаво?
А>>Шаблоны — это, конечно, круто. Но не в этом случае. Принципиальная проблема с шаблонами заключается в том, что они создаются во время компиляции, а набор блокируемых ресурсов во время компиляции может быть и неизвестен.
SH>Т.е ты динамически определяешь, что где блокировать, а где нет? Круто. Я представляю, как это сделать, но пока не могу придумать соответствующей задачи. Да, для такого сдучая мой вариант не подходит.
Уточняю: я о случае, когда существует несколько однородных, не независимых друг от друга ресурсов (см. пример выше).
А>>Кроме того, даже если набор таких ресурсов в конкретном приложении статичен, подход с "волшебным" числом неверен по большому счету хотя бы потому, что в крупном проекте приведет к путанице, какие "волшебства" уже назначены, а какие нет.
SH>typedef не поможет? Меня устаривает..
Когда много типов таких ресурсов, это придется где-то сводить в одном месте.
А>>Функции разблокировки удобнее просто разделить на UnlockReading и UnlockWriting, не требуя хранения флага m_write.
SH>Так это помнить надо. Я отказался от функций блокировки/разблокировки как раз потому, что с ними нужно не забыть разблокировать. Так написал в начале функции SuperLocker lock(false) и больше ни о чём не думаешь. Не нужно на всех путях выхода из функции (в том числе и на исключениях) ставить lock.Unlock или нечто подобное.
Правильно — я и написал: два отдельных класса блокировок, тем более, не нужен флаг.
Я заново прочитал, то что понаписал тебе и понял, что был сильно неправ. Приношу свои извенения. За тон, сарказм и т.п. Что-то во мне заиграло вдруг, захотелось как-то так ответить.. Даже не знаю на что.. Надеюсь, ты не обиделся.
Если по сути, то методом постепенных приближений я пришёл к тому, что рекомендовал Аноним.
Здравствуйте SergH, Вы писали:
SH>Здравствуйте Алекс, Вы писали:
SH>Я заново прочитал, то что понаписал тебе и понял, что был сильно неправ. Приношу свои извенения. За тон, сарказм и т.п. Что-то во мне заиграло вдруг, захотелось как-то так ответить.. Даже не знаю на что.. Надеюсь, ты не обиделся.
Да, нормально. Недавно в WinAPI меня так уделали (на пустом месте) .
SH>Если по сути, то методом постепенных приближений я пришёл к тому, что рекомендовал Аноним.