SRC: Locker2
От: Алекс Россия http://wise-orm.com
Дата: 19.09.02 04:41
Оценка:
Всем привет!

Навеяно предыдущим топиком. Сначала еще раз покритикую код SergH
В конструкторе
     m_write = write;               //1

        if (!m_write)
        {
            WaitForSingleObject(mutex, INFINITE);        //2


между 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);
    };

    void Done()
    {
        if (!m_write)
        {
            WaitForSingleObject(mutex, INFINITE);
            if (!InterlockedDecrement(&readers))
                SetEvent(event);            
            ReleaseMutex(mutex);
        }
        else
            ReleaseMutex(mutex);
    };

    ~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;


Критика принимается.
Re: SRC: Locker2
От: Аноним  
Дата: 19.09.02 05:41
Оценка:
class CSyncError {};

class CWriteGuard  
{
    HANDLE m_hMutexNoWriter;
    HANDLE m_hEventNoReaders;

    volatile DWORD m_dwReaders;

public:
    CWriteGuard() : m_dwReaders(0)
    {
        m_hMutexNoWriter = ::CreateMutex(NULL, FALSE, NULL);
    
        if (!m_hMutexNoWriter)
            throw CSyncError();

        m_hEventNoReaders = ::CreateEvent(NULL, TRUE, TRUE, NULL);

        if (!m_hEventNoReaders)
        {
            ::CloseHandle(m_hMutexNoWriter);
            throw CSyncError();
        }
    }

    ~CWriteGuard()
    {
        ::CloseHandle(m_hMutexNoWriter);
        ::CloseHandle(m_hEventNoReaders);
    }

    void LockWriting()
    {
        HANDLE aHandles[2];

        aHandles[0] = m_hEventNoReaders;
        aHandles[1] = m_hMutexNoWriter;

        ::WaitForMultipleObjects(2, aHandles, TRUE, INFINITE);
    }

    bool LockWriting(DWORD dwTimeOut)
    {
        HANDLE aHandles[2];

        aHandles[0] = m_hEventNoReaders;
        aHandles[1] = m_hMutexNoWriter;

        DWORD dwWaitResult = ::WaitForMultipleObjects(2, aHandles, TRUE, dwTimeOut);

        return dwWaitResult != WAIT_TIMEOUT;
    }

    void UnlockWriting()
    {
        ::ReleaseMutex(m_hMutexNoWriter);
    }

    void LockReading()
    {
        ::WaitForSingleObject(m_hMutexNoWriter, INFINITE);

        if (!m_dwReaders++)
            // Если это первый чтец, сбрасываем событие "нет чтецов"
            ::ResetEvent(m_hEventNoReaders);

        ::ReleaseMutex(m_hMutexNoWriter);
    }

    bool LockReading(DWORD dwTimeOut)
    {
        DWORD dwWaitResult = ::WaitForSingleObject(m_hMutexNoWriter, dwTimeOut);

        if (dwWaitResult == WAIT_TIMEOUT)
            return false;

        if (!m_dwReaders++)
            ::ResetEvent(m_hEventNoReaders);

        ::ReleaseMutex(m_hMutexNoWriter);

        return true;
    }

    void UnlockReading()
    {
        ::WaitForSingleObject(m_hMutexNoWriter, INFINITE);

        if (!--m_dwReaders)
            ::SetEvent(m_hEventNoReaders);

        ::ReleaseMutex(m_hMutexNoWriter);
    }

    class CWriteLock
    {
        CWriteGuard& m_ws;
        bool m_fLocked;

    public:
        CWriteLock(CWriteGuard& ws, bool fInitiallyLocked = true) : m_ws(ws), m_fLocked(false)
        {
            if (fInitiallyLocked)
                Lock();
        }

        ~CWriteLock()
        {
            Unlock();
        }

        void Lock()
        {
            if (!m_fLocked)
                m_ws.LockWriting();

            m_fLocked = true;
        }

        bool Lock(DWORD dwTimeOut)
        {
            return m_fLocked || (m_fLocked = m_ws.LockWriting(dwTimeOut));
        }

        void Unlock()
        {
            if (m_fLocked)
                m_ws.UnlockWriting();

            m_fLocked = false;
        }
    };

    class CReadLock
    {
        CWriteGuard& m_ws;
        bool m_fLocked;

    public:
        CReadLock(CWriteGuard& ws, bool fInitiallyLocked = true) : m_ws(ws), m_fLocked(false)
        {
            if (fInitiallyLocked)
                Lock();
        }

        ~CReadLock()
        {
            Unlock();
        }

        void Lock()
        {
            if (!m_fLocked)
                m_ws.LockReading();

            m_fLocked = true;
        }

        bool Lock(DWORD dwTimeOut)
        {
            return m_fLocked || (m_fLocked = m_ws.LockReading(dwTimeOut));
        }

        void Unlock()
        {
            if (m_fLocked)
                m_ws.UnlockReading();

            m_fLocked = false;
        }
    };
};
Re: SRC: Locker2
От: Алекс Россия http://wise-orm.com
Дата: 19.09.02 05:47
Оценка:
Здравствуйте Алекс, Вы писали:

[]

А>Критика принимается.


блин в функции Want2Read() конечно же ResetEvent() нужно поставить.
И намек Анонима я понял. Действительно от InterlockedXXX функций можно отказаться.
Re[2]: SRC: Locker2
От: Алекс Россия http://wise-orm.com
Дата: 19.09.02 05:50
Оценка:
Здравствуйте Алекс, Вы писали:

[]

Вот так получается
    void Want2Write()
    {
        HANDLE h[] = {event, mutex};
        WaitForMultipleObjects(2, h, TRUE, INFINITE);
        m_write = true;
    };

    void Want2Read()
    {
        WaitForSingleObject(mutex, INFINITE);
    
        m_write = false;

        if (!readers)
            ResetEvent(event);
        Readers++;
        ReleaseMutex(mutex);
    };

    void Done()
    {
        if (!m_write)
        {
            WaitForSingleObject(mutex, INFINITE);
            if (!--readers)
                SetEvent(event);            
        }
        ReleaseMutex(mutex);
    };
Re[2]: SRC: Locker2
От: Аноним  
Дата: 19.09.02 06:15
Оценка:
Этот код реально применяется в работе.

Теперь критика других вариантов.

Принципиально, по архитектуре — необходимо разделение двух типов объектов: охраняемого ресурса и блокировок.

То, что члены класса объявлены как статические, — нехорошо (даже если оставить в стороне вопиющие проблемы с инициализацией и разрушением объектов ядра). Шаблоны — это, конечно, круто. Но не в этом случае. Принципиальная проблема с шаблонами заключается в том, что они создаются во время компиляции, а набор блокируемых ресурсов во время компиляции может быть и неизвестен. Кроме того, даже если набор таких ресурсов в конкретном приложении статичен, подход с "волшебным" числом неверен по большому счету хотя бы потому, что в крупном проекте приведет к путанице, какие "волшебства" уже назначены, а какие нет.

Далее, счетчик читателей должен быть объявлен как volatile, т. к. все функции довольно просты, легко встраиваются и у компилятора может возникнуть страстное желание соптимизировать эту переменную.

Никакой нужды использовать Interocked-функции для счетчика чтецов.

Функции разблокировки удобнее просто разделить на UnlockReading и UnlockWriting, не требуя хранения флага m_write.

Не против критики в свой адрес.
Re[3]: SRC: Locker2
От: orangy Россия
Дата: 19.09.02 06:25
Оценка:
Здравствуйте , Вы писали:

>Функции разблокировки удобнее просто разделить на UnlockReading и UnlockWriting, не требуя хранения флага m_write.

Или использовать m_dwReaders (сделав int-ом) со отрицательным значением для писателей.
... << J 1.0 alpha 4 >>
"Develop with pleasure!"
Re: SRC: Locker2
От: SergH Россия  
Дата: 19.09.02 11:13
Оценка:
Здравствуйте Алекс, Вы писали:

А>Навеяно предыдущим топиком. Сначала еще раз покритикую код SergH

А>В конструкторе
А>
А>     m_write = write;               //1

А>        if (!m_write)
А>        {
А>            WaitForSingleObject(mutex, INFINITE);        //2
А>


А>между 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;
Делай что должно, и будь что будет
Re[3]: SRC: Locker2
От: SergH Россия  
Дата: 19.09.02 11:25
Оценка:
Здравствуйте Аноним, Вы писали:

А>Теперь критика других вариантов.


А>Принципиально, по архитектуре — необходимо разделение двух типов объектов: охраняемого ресурса и блокировок.


Ты имеешь ввиду, разделение на CWriteGuard и CReadGuard? Мне не очевидна необходимсть такого разделения.

А>То, что члены класса объявлены как статические, — нехорошо (даже если оставить в стороне вопиющие проблемы с инициализацией и разрушением объектов ядра).


А по мне хорошо. Нужно было просто ввести дополнительный уровень абстакции — сделать статический объект на подобии твоего, а в конструкторе вызывать LockRead\LockWrite..

А>Шаблоны — это, конечно, круто. Но не в этом случае. Принципиальная проблема с шаблонами заключается в том, что они создаются во время компиляции, а набор блокируемых ресурсов во время компиляции может быть и неизвестен.


Т.е ты динамически определяешь, что где блокировать, а где нет? Круто. Я представляю, как это сделать, но пока не могу придумать соответствующей задачи. Да, для такого сдучая мой вариант не подходит.

А>Кроме того, даже если набор таких ресурсов в конкретном приложении статичен, подход с "волшебным" числом неверен по большому счету хотя бы потому, что в крупном проекте приведет к путанице, какие "волшебства" уже назначены, а какие нет.


typedef не поможет? Меня устаривает..

А>Далее, счетчик читателей должен быть объявлен как volatile, т. к. все функции довольно просты, легко встраиваются и у компилятора может возникнуть страстное желание соптимизировать эту переменную.


Согласен, исправлюсь.

А>Никакой нужды использовать Interocked-функции для счетчика чтецов.


Согласен.

А>Функции разблокировки удобнее просто разделить на UnlockReading и UnlockWriting, не требуя хранения флага m_write.


Так это помнить надо. Я отказался от функций блокировки/разблокировки как раз потому, что с ними нужно не забыть разблокировать. Так написал в начале функции SuperLocker lock(false) и больше ни о чём не думаешь. Не нужно на всех путях выхода из функции (в том числе и на исключениях) ставить lock.Unlock или нечто подобное.
Делай что должно, и будь что будет
Re[2]: SRC: Locker2
От: Алекс Россия http://wise-orm.com
Дата: 19.09.02 11:30
Оценка:
Здравствуйте SergH, Вы писали:

SH>Здравствуйте Алекс, Вы писали:


А>>Навеяно предыдущим топиком. Сначала еще раз покритикую код SergH

А>>В конструкторе
А>>
А>>     m_write = write;               //1

А>>        if (!m_write)
А>>        {
А>>            WaitForSingleObject(mutex, INFINITE);        //2
А>>


А>>между 1 и 2 может случиться переключение контекста, тогда будет жопа


SH>Объясни, не понял.


ну вот ты записал в переменную например 1. Случилось переключение контекста, другой поток получил управление и хочет читать. Он туда записал 0. После этого твой поток снова получил управление. И тут условие
if (!m_write)

а m_write содержит уже неверное значение.

хъ

SH>Это у тебя каждый раз статические объекты переинициализируются? Круто, круто...Смело. Или ты думаешь, что просто увеличиваешь счётчик ссылок объекта? Так ведь нет.. Хотя, можно использовать именованные объекты, тогда будет да.. Но значение описателя всё равно будет не такое, т.к. понятие "счётчик ссылок описателя" не существует...


Да, лоханулся, исправленный вариант лежит здесь
Автор: Алекс
Дата: 19.09.02
.

хъ

SH>А после этого, судя по названию читать.. Но почему readers уменьшается? И почему событие

SH>устанавливается, оно должно сбрасываться

Тоже лоханулся, исправленный вариант смотреть там же.

А>> void Done()

А>> {
А>> if (!m_write)
А>> {
А>> WaitForSingleObject(mutex, INFINITE);
А>> if (!InterlockedDecrement(&readers)) //1
А>> SetEvent(event); //2
А>> ReleaseMutex(mutex);
А>> }
А>> else
А>> ReleaseMutex(mutex);
А>> };

SH>А если переключение произойдёт между 1 и 2?


Ну и пусть. Мы же защищены мутексом. Другой поток эту часть кода не исполнит.

хъ

А>>private:

А>> static HANDLE mutex;
А>> static HANDLE event;
А>> static LONG readers;
А>> bool m_write;
А>>};

Ну понятное дело нужно их объявить локалными. Лоханулся, признаю, с кем не бывает.
Re[3]: SRC: Locker2
От: SergH Россия  
Дата: 19.09.02 11:34
Оценка:
Здравствуйте Алекс, Вы писали:

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?


А>Ну и пусть. Мы же защищены мутексом. Другой поток эту часть кода не исполнит.


Сорри, это я не заметил.
Делай что должно, и будь что будет
Re[2]: SRC: Locker2
От: SergH Россия  
Дата: 19.09.02 11:43
Оценка:
Здравствуйте Алекс, Вы писали:

А>>В деструкторе я бы сначала проверил 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 или нечто подобное.


Правильно — я и написал: два отдельных класса блокировок, тем более, не нужен флаг.
Re[3]: SRC: Locker2
От: SergH Россия  
Дата: 23.09.02 16:13
Оценка:
Здравствуйте Алекс, Вы писали:

Я заново прочитал, то что понаписал тебе и понял, что был сильно неправ. Приношу свои извенения. За тон, сарказм и т.п. Что-то во мне заиграло вдруг, захотелось как-то так ответить.. Даже не знаю на что.. Надеюсь, ты не обиделся.

Если по сути, то методом постепенных приближений я пришёл к тому, что рекомендовал Аноним.
Делай что должно, и будь что будет
Re[4]: SRC: Locker2
От: Алекс Россия http://wise-orm.com
Дата: 24.09.02 09:51
Оценка:
Здравствуйте SergH, Вы писали:

SH>Здравствуйте Алекс, Вы писали:


SH>Я заново прочитал, то что понаписал тебе и понял, что был сильно неправ. Приношу свои извенения. За тон, сарказм и т.п. Что-то во мне заиграло вдруг, захотелось как-то так ответить.. Даже не знаю на что.. Надеюсь, ты не обиделся.


Да, нормально. Недавно в WinAPI меня так уделали (на пустом месте) .

SH>Если по сути, то методом постепенных приближений я пришёл к тому, что рекомендовал Аноним.


Я бы тоже ему балов 12 поставил.
 
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.