Unity программист
0,0
рейтинг
17 октября 2014 в 09:18

Разработка → Потокобезопасные события в C# или Джон Скит против Джеффри Рихтера

C#*, .NET*


Готовился я как-то к собеседованию по C# и среди прочего нашел вопрос примерно следующего содержания:
«Как организовать потокобезопасный вызов события в C# с учетом того, что большое количество потоков постоянно подписываются на событие и отписываются от него?»


Вопрос вполне конкретно и чётко поставлен, поэтому я даже не сомневался в том, что ответ на него можно дать также чётко и однозначно. Но я очень сильно ошибался. Оказалось, что это крайне популярная, избитая, но до сих пор открытая тема. А еще заметил не очень приятную особенность — в русскоязычных ресурсах этому вопросу уделяется очень мало внимания (и хабр не исключение), поэтому я решил собрать всю найденную и переваренную мной информацию по данному вопросу.
До Джона Скита и Джеффри Рихтера тоже дойдем, они то, собственно, и сыграли ключевую роль в моем общем понимании проблемы работы событий в многопоточной среде.

Особо внимательный читатель сможет найти в статье два комикса в стиле xkcd.
(Осторожно, внутри две картинки примерно по 300-400 кб)


Продублирую вопрос, на который надо ответить:

«Как организовать потокобезопасный вызов события в C# с учетом того, что большое количество потоков постоянно подписываются на событие и отписываются от него?»


У меня было предположение, что часть вопросов опирается на книгу CLR via C#, тем более что в моей любимой C# 5.0 in a Nutshell подобный вопрос вообще не рассматривался, поэтому начнем с Джеффри Рихтера (CLR via C#).

Путь Джеффри Рихтера


Небольшая выдержка из написанного:

Долгое время рекомендованным способом вызова событий была примерно следующая конструкция:

Вариант 1:
public event Action MyLittleEvent;
...
protected virtual void OnMyLittleEvent()
{
    if (MyLittleEvent != null) MyLittleEvent();
}


Проблема подобного подхода в том, что в методе OnMyLittleEvent один поток может увидеть, что наше событие MyLittleEvent не равно null, а другой поток, сразу после этой проверки, но перед вызовом события, может убрать свой делегат из списка подписчиков, и таким образом сделать из нашего события MyLittleEvent null, что приведет к выбросу NullReferenceException в месте вызова события.

Вот небольшой комикс в стиле xkcd, который наглядно иллюстрирует эту ситуацию (два потока работают параллельно, время идет сверху вниз):
Развернуть



В целом всё логично, у нас обычное состояние гонки (здесь и далее — race condition). И вот как эту проблему решает Рихтер (и этот вариант встречается чаще всего):

Добавим в наш метод вызова события локальную переменную, в которую будем копировать наше событие на момент «захода» в метод. Поскольку делегаты — это неизменяемые объекты (здесь и далее — immutable), у нас получится «замороженная» копия события, от которой никто уже не сможет отписаться. При отписке от события создается новый объект делегата, который заменяет объект в поле MyLittleEvent, в то время как у нас остается локальная ссылка на старый объект делегата.

Вариант 2:
protected virtual void OnMyLittleEvent()
{
    Action tempAction = MyLittleEvent; // "Заморозили" наше событие для текущего метода
    // На наш tempAction теперь никто не может повлиять, он никогда не станет равен null ни при каких условиях
    if (tempAction != null) tempAction (); 
}


Далее у Рихтера описывается, что JIT компилятор вполне может просто опустить создание локальной переменной ради оптимизации, и сделать из второго варианта первый, то есть пропустить «заморозку» события. В итоге рекомендуется делать копирование через Volatile.Read(ref MyLittleEvent), то есть:

Вариант 3:
protected virtual void OnMyLittleEvent()
{
    // Ну теперь уж точно заморозили
    Action tempAction = Volatile.Read(ref MyLittleEvent); 
    if (tempAction != null) tempAction (); 
}


Про Volatile можно долго говорить отдельно, но в общем случае это «просто позволяет избавиться от нежелательной оптимизации JIT компилятора». По этому поводу еще будут уточнения и подробности, но мы пока остановимся на общей идее текущего решения Джеффри Рихтера:

Для обеспечения потокобезопасного вызова события нужно «заморозить» текущий список подписчиков, скопировав событие в локальную переменную, после чего, в случае если полученный список не пустой, вызвать все обработчики из «замороженного» списка. Таким образом мы избавляемся от возможного NullReferenceException.


Меня сразу смутило то, что мы вызываем события у уже отписавшихся объектов/потоков. Вряд ли кто-то отписался просто так — вполне вероятно, что кто-то сделал это во время общей «чистки» следов — вместе с закрытием потоков записи / чтения (например логгер, который по событию должен был писать данные в файл), закрытием соединений и т.п., то есть внутреннее состояние объекта-подписчика на момент вызова его обработчика может быть непригодно для дальнейшей работы.
Для примера представим, что наш подписчик реализует метод IDisposable, и следует конвенции, определяющей что при попытке вызвать любой метод у освобожденного (здесь и далее — disposed) объекта, он должен выбросить ObjectDisposedException. Также условимся, что мы отписываемся от всех событий в методе Dispose.
А теперь представте такой сценарий — мы вызываем у этого объекта метод Dispose ровно после того момента, когда другой поток «заморозил» список своих подписчиков. Поток успешно вызывает обработчик у отписавшегося объекта, а тот во время попытки обработки события объект рано или поздно понимает, что он уже был освобожден, и выбрасывает ObjectDisposedException. Скорее всего это исключение в самом обработчике никак не ловится, потому что вполне логично предположить: «Если наш подписчик отписался и был освобожден, то его обработчик никогда не будет вызван». Тут либо будет краш приложения, либо утечка неуправляемых ресусов, либо вызов события прервется при первом появлении ObjectDisposedException (если мы ловим исключение при вызове), но до нормальных «живых» обработчиков событие так и не доберется.

Вернемся к комиксу. История та же — два потока, время идет сверху вниз. Вот что происходит на самом деле:
Развернуть



Эта ситуация, по-моему, намного серьезней, чем возможное NullReferenceException при вызове события.
Что интересно, советы по реализации потокобезопасного вызова событий на стороны Наблюдаемого объекта есть, а советов по реализации потокобезопасных Обработчиков — нет.

О чем говорит StackOverflow


На SO можно найти подробную «статью» (да, вопрос этот тянет на целую небольшую статью), посвященную данному вопросу.

В целом там разделяется моя точка зрения, но вот что добавляет этот товарищ:

Мне кажется, что вся эта шумиха с локальными переменными — ничто иное, как Карго-культ программирование (Cargo Cult Programming). Большое количество людей решает проблему потокобезопасных событий именно таким способом, в то время как для полноценной потокобезопасности нужно сделать намного больше. Я могу с уверенностью сказать, что те люди, которые не добавляют в свой код подобные проверки, могут вполне обойтись без них. Этой проблемы просто не существует в однопоточном окружении, да и учитывая что в онлайн примерах с кодом редко можно встретить ключевое слово volatile, эта дополнительная проверка вполне может быть бессмысленной. Если нашей задачей является отслеживание NullReferenceException, нельзя ли обойтись вообще без проверки на null, присвоив пустой delegate { } нашему событию во время инициализации объекта класса?


Это подводит нас к еще одному варианту решения проблемы.

public event Action MyLittleEvent = delegate {};


MyLittleEvent никогда не будет равно null, и лишнюю проверку можно просто не делать. В многопоточной среде нужно только синхронизировать добавление и удаление подписчиков события, но вызывать его можно без опасения получения NullReferenceException:

Вариант 4:
public event Action MyLittleEvent = delegate {};

protected virtual void OnMyLittleEvent()
{
    // Собственно, это все
    MyLittleEvent();
}


Единственный минус такого подхода по сравнению с предыдущим — небольшой оверхед на вызов пустого события (оверхед оказался равен примерно 5 наносекунд на вызов). Можно также подумать, что в случае большого количества разных классов с разными событиями, эти пустые «затычки» для событий будут занимать много места в оперативной памяти, но если верить Джону Скиту в ответе на SO, начиная с версии C# 3.0 компилятор использует один и тот же объект пустого делегата для всех «затычек». От себя добавлю, что при проверке получившегося IL кода это утверждение не подтверждается, пустые делегаты создаются по штуке на событие (проверял с помощью LINQPad и ILSpy). В крайнем случае можно сделать общее на проект статическое поле с пустым делегатом, к которому можно обращаться из всех участков программы.

Путь Джона Скита



Раз уж мы добрались до Джона Скита, стоит отметить его реализацию потокобезопасных событий, которую он описал в C# in Depth в разделе Delegates and Events (статья онлайн и перевод товарища Klotos)

Суть в том, чтобы закрыть add, remove и локальную «заморозку» в lock, что позволит избавиться от возможных неопределенностей с одновременной подпиской на событие нескольких потоков:

Немного кода
SomeEventHandler someEvent;

readonly object someEventLock = new object();

public event SomeEventHandler SomeEvent
{
    add
    {
        lock (someEventLock)
        {
            someEvent += value;
        }
    }
    remove
    {
        lock (someEventLock)
        {
            someEvent -= value;
        }
    }
}

protected virtual void OnSomeEvent(EventArgs e)
{
    SomeEventHandler handler;
    lock (someEventLock)
    {
        handler = someEvent;
    }
    if (handler != null)
    {
        handler (this, e);
    }
} 



Несмотря на то, что этот метод считается устаревшим (внутренняя реализация событий начиная с C# 4.0 выглядит совершенно по-другому, см. список источников в конце статьи), он наглядно показывает, что нельзя просто обернуть вызов событий, подписку и отписку в lock, поскольку это с очень большой вероятностью может привести к взаимоблокировке (здесь и далее — deadlock). В lock находится только копирование в локальную переменную, сам вызов события происходит вне этой конструкции.

Но это совершенно не решает проблему вызова обработчиков у уже отписавшихся событий.

Вернемся к вопросу на SO. Дэниель, в ответ на все наши способы предотвращения NullReferenceException высказывает очень интересную мысль:

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


Среди прочих, на вопрос ответил и Джон Скит, и вот что он пишет.

Джон Скит против Джеффри Рихтера



JIT компилятор не имеет права оптимизировать локальную ссылку на делегат, поскольку там присутствует условие. Эту информацию «вбросили» некоторое время назад, но это неправда (я уточнял этот вопрос то ли у Джо Даффи, то ли у Ванса Мориссона). Без модификатора volatile просто возникает возможность, что локальная ссылка на делегат будет немного устаревшей, но в целом это все. Это не приведет к NullReferenceException.

И да, у нас определенно возникает состояние гонки, вы правы. Но оно будет присутствовать всегда. Допустим, мы уберем проверку на null и просто напишем
MyLittleEvent();

А теперь представьте, что наш список подписчиков состоит из 1000 делегатов. Вполне возможно, что мы начнем вызывать событие перед тем, как один из подписчиков отпишется от него. В этом случае он все равно будет вызван, поскольку он останется в старом списке (не забывайте, что делегаты неизменяемы). Насколько я понимаю, это совершенно неизбежно.
Использование пустого delegate {}; избавляет нас от необходимости проверять событие на null, но это не спасет нас от очередного состояния гонки. Более того, этот способ не гарантирует, что мы будем использовать наиболее свежую версию события.


Теперь надо отметить, что этот ответ был написан в 2009м году, а CLR via C# 4th edition — в 2012. Так кому в итоге верить?
На самом деле я не понял, зачем Рихтер описывает случай с копированием в локальную переменную через Volatile.Read, поскольку дальше он подтверждает слова Скита:
Хоть и рекомендуется использовать версию с Volatile.Read как наилучшую и технически верную, можно обойтись и Вариантом 2, поскольку JIT компилятор знает о том, что он может случайно натворить, оптимизируя локальную переменную tempAction. Чисто теоретически, в будущем это может измениться, поэтому рекомендуется использовать Вариант 3. Но на самом деле Microsoft вряд ли пойдет на такие изменения, поскольку это может сломать огромное количество уже готовых программ.


Всё становится совершенно запутанным — оба варианты равнозначны, но тот, что с Volatile.Read более равнозначен. И ни один вариант не спасет от состояния гонки при вызове отписавшихся обработчиков.

Может потокобезопасного способа вызова событий вообще не существует? Почему на предотвращение маловероятного NullReferenceException тратится столько сил и времени, а на предотвращение не менее вероятного вызова отписавшегося обработчика — нет? Этого я так и не понял. Но зато в процессе поиска ответов я понял много всего другого, и вот небольшой итог.

Что имеем в итоге



  • Наиболее популярный способ не является потокобезопасным из-за возможности превращения делегата в null после поверки на неравенство. Появляется опасность возникновения NullReferenceException
    public event Action MyLittleEvent;
    ...
    protected virtual void OnMyLittleEvent()
    {
        if (MyLittleEvent != null) 
                // Опасность NullReferenceException
                MyLittleEvent();
    }
    

  • Методы Скита и Рихтера помогают избежать возникновения NullReferenceException, но не являются потокобезопасными, поскольку остается вероятность вызова уже отписавшихся обработчиков.
    Метод Скита
    SomeEventHandler someEvent;
    
    readonly object someEventLock = new object();
    
    public event SomeEventHandler SomeEvent
    {
        add
        {
            lock (someEventLock)
            {
                someEvent += value;
            }
        }
        remove
        {
            lock (someEventLock)
            {
                someEvent -= value;
            }
        }
    }
    
    protected virtual void OnSomeEvent(EventArgs e)
    {
        SomeEventHandler handler;
        lock (someEventLock)
        {
            handler = someEvent;
        }
        if (handler != null)
        {
            handler (this, e);
        }
    } 
    


    Метод Рихтера
    protected virtual void OnMyLittleEvent()
    {
        // Ну теперь уж точно заморозили
        Action tempAction = Volatile.Read(ref MyLittleEvent); 
        if (tempAction != null) tempAction (); 
    }
    


  • Метод пустого delegate {}; позволяет избавиться от NullReferenceException благодаря тому, что событие никогда не обращается в null, но не является потокобезопасным поскольку остается вероятность вызова уже отписавшихся обработчиков. Более того, без модификатора volatile у нас есть возможность получить не самую свежую версию делегата при вызове события.
  • Нельзя просто обернуть добавление, удаление и вызов события в lock, поскольку это создаст опасность взаимоблокировки. Технически, это может спасти от вызова отписавшихся обработчиков, но мы не можем быть уверенными в том, какие действия сделал объект-подписчик перед тем, как отписаться от события, поэтому мы все еще можем напороться на «испорченный» объект (см. пример с ObjectDisposedException). Этот метод также не является потокобезопасным.
  • Попытка поймать отписавшиеся делегаты после локальной «заморозки» события бессмысленна — при большом количестве подписчиков вероятность вызова отписавшихся обработчиков (после начала вызова события) даже выше, чем при локальной «заморозке».


Технически, ни один из представленных вариантов не является потокобезопасным способом вызова события. Более того, добавление метода проверки делегата с помощью локальных копий делегатов создает ложное чувство защищенности. Единственным способом, позволяющим полностью обезопасить себя — заставить обработчики событий проверять, не отписались ли они уже от конкретного события. К сожалению, в отличие от общих практик предотвращения NullReferenceException при вызове событий, по поводу обработчиков нет никаких предписаний. Если вы делаете отдельную библиотеку, то чаще всего вы никак не можете повлиять на её пользователей — не можете заставить клиентов предполагать, что их обработчики не будут вызваны после отписки от события.

После осознания всех этих проблем у меня остались смешанные чувства по поводу внутренней реализации делегатов в C#. С одной стороны, поскольку они являются неизменяемыми, нет шансов получить InvalidOperationException как в случае перебора изменяющейся коллекции через foreach, но с другой — нет никакой возможности проверить, отписался ли кто-то от события во время вызова или нет. Единственное что можно сделать со стороны держателя события — обезопаситься от NullReferenceException и надеяться, что подписчики ничего не испортят. В итоге на поставленный вопрос можно ответить так:

Невозможно обеспечить потокобезопасный вызов события в многопоточном окружении, поскольку всегда остается вероятность вызова обработчиков уже отписавшихся подписчиков. Эта неопределенность противоречит определению термина «потокобезопасность», в частности пункту
Implementation is guaranteed to be free of race conditions when accessed by multiple threads simultaneously.



Дополнительное чтение


Разумеется, я не мог просто скопировать / перевести всё, что нашел. Поэтому оставлю список источников, которые были прямо или косвенно использованы.

Кирилл Надеждин @KumoKairo
карма
32,0
рейтинг 0,0
Unity программист
Реклама помогает поддерживать и развивать наши сервисы

Подробнее
Спецпроект

Самое читаемое Разработка

Комментарии (120)

  • +7
    Не вижу проблемы в вызове уже отписанного обработчика. Сейчас поясню, почему.

    Рассмотрим такой код:
    foo.SomeEvent += SomeHandler;
    ...
    foo.SomeEvent -= SomeHandler;
    done = true;
    
    //
    
    void SomeHandler() {
        if (done) {
            throw new InvalidOperationException("Эй, я уже отписан!!!");
        }
    }
    
    Фактически, любой код, в котором возникает проблема вызова обработчика после отписки от события, чем-то похож на приведенный мною: всегда есть некоторое условие, которое появляется после отписки — и в зависимости от которого происходит ошибка в обработчике.

    Теперь рассмотрим две ситуации.
    Ситуация первая:
    1. Сработало событие
    2. Делегат был сохранен в локальную переменную
    3. В другом потоке выполнились строчки foo.SomeEvent -= SomeHandler; done = true;
    4. Был вызван обработчик SomeHandler, который в итоге и вылетел
    Это — та самая ситуация, которой боится автор.

    Ситуация вторая:
    1. Сработало событие
    2. Был вызван обработчик SomeHandler, однако поток был прерван прежде, чем он дошел до условного оператора
    3. В другом потоке выполнились строчки foo.SomeEvent -= SomeHandler; done = true;
    4. Обработчик SomeHandler в итоге вылетел
    Это — состояние гонки, которое никак не зависит от внутренней реализации объекта foo.

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

    Могу переформулировать свое утверждение: в данном коде на самом деле происходит гонка между присваиванием переменной done и ее проверкой — а отписка от события лишь маскирует эту гонку. Если решить проблему этой гонки — то и проблема вызова отписанного обработчика не страшна.

    PS Мой любимый способ безопасного вызова события:
    public static partial class Extensions {
        public static void SafeInvoke(this EventHandler action, EventArgs e) {
            if (action != null) action(e);
        }
    }
    
    ...
    
    protected virtual void OnSomeEvent(EventArgs e) {
        SomeEvent.SafeInvoke();
    }
    
    Уж параметр метода-то ни при каких оптимизациях измениться между проверкой и вызовом не должен…
    • +8
      Уж параметр метода-то ни при каких оптимизациях измениться между проверкой и вызовом не должен…
      А ещё JIT умеет инлайнить методы
      • 0
        удалено
      • +3
        А можно использовать
        [MethodImpl(MethodImplOptions.NoInlining)]
        
    • +3
      В целом своим комментарием просто соглашаетесь с моим итогом:
      А следовательно, защита от вызова события после отписывания от него находится в области ответственности внешнего кода.

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

      Об этом я написал в конце статьи.

      К сожалению, проверка внутреннего состояния внутри обработчика нигде не документирована и не является общепринятой практикой.

      А поскольку мы не можем быть уверенными в потокобезопасной реализации обработчиков, общее решение также не является потокобезопасным.
    • 0
      Вы заменили гонку из статьи собственно придуманной гонкой, но суть проблемы это не меняет. Опять же можно в вашем варианте попробовать залокировать отписку и операции с переменной done, но и здесь, аналогично, как в статье, всплывает проблема deadlock. Чисто техническая замена внутренней проверки на проверку извне ничего не даст.
      • +1
        Да, об этом я тоже написал
      • 0
        Почему же не даст?
        foo.SomeEvent += SomeHandler;
        ...
        foo.SomeEvent -= SomeHandler;
        done = true;
        
        //
        
        void SomeHandler() {
            if (done) return;
        }
        


        Можно даже lock поставить, безо всяких взаимоблокировок получится!
        foo.SomeEvent += SomeHandler;
        ...
        foo.SomeEvent -= SomeHandler;
        lock(some_lock) {
            done = true;
        }
        
        //
        
        void SomeHandler() {
            lock(some_lock) {
                if (done) return;
            }
        }
        


        Чисто техническая замена внутренней проверки на проверку извне ничего не даст.
        А потому что не надо менять «чисто технически». Надо думать о том, какие могут возникнуть проблемы — и именно их и решать.
        • +3
          Проблема в том, что вы не можете заставить все обработчики проверять, отписались ли они уже от обработчика или нет. В отличие от общей практики проверки на NullReferenceException, практика проверки не является документированной и общепринятой. Поскольку потокобезопасность в данном случае зависит от реализации обработчика — этот метод нельзя назвать полностью потокобезопасным.
          • –3
            Но это уже не моя проблема. Если кто-то пишет потоконебезопасный код — то он сам виноват.
            • +2
              Если бы всё было так просто…

              Вопрос был в том, можно ли обеспечить потокобезопасный вызов события. Ответ — нет, поскольку потокобезопасность в данном случае зависит не только от реализации вызова, но и от реализации конкретного обработчика, на который мы никак не можем повлиять.
              • 0
                У нас есть две операции: отписка от события — и вызов всех обработчиков, выполняющиеся в разных потоках одновременно.
                1. SomeEvent -= SomeHandler
                2. OnSomeEvent()

                Если первая операция завершается раньше, чем мы прочитаем значение поля — то обработчик SomeHandler не будет вызван, как будто мы вызвали сначала операцию 1, а потом операцию 2 в одном и том же потоке.
                В противном случае, обработчик будет вызван — как будто мы вызвали в одном и том же сначала операцию 2, а потом операцию 1.

                В обоих случаях параллельный вызов операций эквивалентен их последовательному вызову в некотором порядке. Такое свойство объекта называется линеаризуемостью. А линеаризуемый код, согласно Википедии, считается потокобезопасным.
                • 0
                  Ну да, я понял вашу точку зрения. Только мне такая потокобезопасность несколько ночей спать не давала… (если что — я вас не минусовал)
        • 0
          Что произойдет, если поток 1 будет отписывать событие и отпишется: foo.SomeEvent -= SomeHandler; но еще не зайдет в критическую секцию, а в это время будет вызов из другого потока вашего SomeHandler? Что даст ваша критическая секция?
          • 0
            Данная критическая секция защищает переменную done. В реальном коде в ней будет работа с общими ресурсами, которые, собственно, и нуждаются в защите.

            Если первый поток успеет отписаться, но не успеет зайти в критическую секцию, то ничего страшного не случится — программа будет работать так, как будто он не успел отписаться.
            • 0
              Причем тут что-то разделяемое? Идеальная задача — сделать атомарную операцию отписки и сброса всех вызовов события. Зачем пихать все в критическую секцию, да и вообще, причем тут разделяемые ресурсы?
              • 0
                Операция отписки атомарна. Операция вызова события — тоже.

                А разделяемые ресурсы тут при том, что в отсутствие разделяемых ресурсов никаких гонок не возникает:
                foo.SomeEvent += SomeHandler;
                ...
                foo.SomeEvent -= SomeHandler;
                
                //
                
                void SomeHandler() {
                    Console.WriteLine("Hello, world!");
                }
                

                К примеру, в этом коде нам совершенно не важно, будет или нет вызван метод SomeHandler в случае одновременной отписки от события и его возникновения.
                • 0
                  Я не совсем к этому, я к тому, что не надо включать разделяемые ресурсы в Lock данном случае. Достаточно включить флаг и потом его в локе проверить. Зачем критическую секцию кидать на все?
                  • 0
                    using (var res = new SomeResource()) {
                        var done = false;
                        var _lock = new object();
                        Action SomeHandler() = () => {
                            lock(_lock) if (!done) res.DoSomething();
                        };
                    
                        foo.SomeEvent += SomeHandler;
                        ...
                        foo.SomeEvent -= SomeHandler;
                        lock(_lock) done = true;
                    }
                    
                    Попробуйте переписать, вызвав res.DoSomething(); вне критической секции.
                    • 0
                      Я о нижнем локе, а не локе в экшене. Плюс преепишите lock(_lock) if(done) return; res.Do...(); и вот вам Do без критической секции?
                      • 0
                        Если сделать любую из предложенных вами правок — возникнет возможность выхода из блока using в то время, когда в другом потоке выполняется res.DoSomething()
                        • 0
                          Да, тут я с вами полностью согласен.
    • +1
      А следовательно, защита от вызова события после отписывания от него находится в области ответственности внешнего кода.

      Насколько я понимаю, внешний код не может полностью защитить от вызова отписавшегося делегата.
      • +1
        Все верно, об этом вся моя статья.

        Внешний код никак не может повлиять на сам факт вызова конкретного обработчика, он может только проверить, правильно ли его вызвали.
      • 0
        Напротив, именно он и может. Потому что в самой «отписке» нет ничего важного, проблема есть лишь в общих ресурсах между кодом, выполняющим отписку — и самим обработчиком. Их-то и надо защищать.

        Если у нас есть обработчик вида void SomeHandler() { Log.Trace("Handled!"); } (то есть не имеющий никаких общих незащищенных ресурсов) — то нам вообще без разницы, вовремя мы его отписываем или нет.
        • 0
          Всё верно, но это слишком частный случай.

          Разумеется, не все отписавшиеся обработчики будут иметь испорченное внутреннее состояние. Но поскольку имеет место неопределенность — мы не можем назвать подобное решение потокобезопасным.
        • +1
          Да, но мы не всегда знаем что находиться «внутри» или «снаружи». Так что было бы хорошо иметь способ полностью себя обезопасить имея контроль лишь над одной из сторон. Но из за природы системы событий в .net это не возможно.
          Совсем другая ситуация когда мы пишем как «вызывающий» так и «обрабатывающий» код, тогда да, все в наших руках.
          • 0
            Так что было бы хорошо иметь способ полностью себя обезопасить имея контроль лишь над одной из сторон.
            Так в чем проблема? Проверка локальной переменной на null делает код совершенно безопасным для нас.

            Зачем пытаться обезопасить «наружний» код, имея контроль лишь над внутренним?
            • +1
              Ниже писали про способы асинхронной передачи сообщений подписчикам, по принципу fire-and-forget. Это будет действительно безопасным способом реализации системы подписчиков.

              Я отметил, что проверка на null дает ложное чувство безопасности, ощущение что теперь с кодом ничего не случится. Тут важно иметь небольшую тревожную заботу по поводу возможных нестыковок.
              • 0
                Асинхронная передача сообщений подписчикам проблему тоже не решает. Разве что отписку и обработку события в одном и том же потоке проводить…
                • 0
                  Ну, почему же? Если рассматривать событие, как «сообщение», то асинхронная отправка сообщения через некую шину проблему решает. Если кто-то отписался, значит сообщение не дойдёт к такому подписчику.
                  • 0
                    Проблема возникает, не когда кто-то отписался, а потом возникло событие. Проблема возникает, когда кто-то отписался в процессе обработки события.
                    • +2
                      Так вот в случае fire-and-forget мы выкинули сообщение и не знаем про подписчиков. Т.е. для нас отправка сообщения атомарна. Мы просто кладём его в очередь.

                      Дальше, со стороны подписчиков получение события тоже безопасно: если сообщение есть, то запрашиваем копию и обрабатываем его.
                      • 0
                        Ха! Рассмотрим такой вот псевдокод:

                        using (var res = new SharedResource()) {
                            var handler = new MessageHandler(res);
                            
                            messaging.Subscribe(foo.SomeEvent, handler);
                        
                            ...
                        
                            messaging.Unsubscribe(foo.SomeEvent, handler);
                        }
                        
                        // где-то внутри MessageHandler
                        void OnMessage() {
                            res.DoSomething();
                        }
                        


                        Вот что нужно сделать, чтобы метод OnMessage никогда не упал с ObjectDisposedException?
                        • +1
                          Так у вас всё та же подписка на события, коллбэки и прочее. Это ни чем концептуально не отличается от механизма event-ов, о которых говорится в статье. А выше предлагают другой механизм, без коллбэков. Где push-модель заменяется на pull (подписчик сам берёт сообщение из очереди, если оно ему нужно). Получается, что у наблюдаемого объекта нет коллекции колбэков, а это значит, что никто обработчик события не вызовет, если наблюдатель этого уже не хочет.
                          • 0
                            Вы правда думаете, что в pull-модели некуда засунуть общий ресурс, при доступе к которому произойдет ошибка?

                            Код не привожу исключительно потому что не знаю, как оно должно выглядеть — ни разу не работал с pull-моделью.
                            • +2
                              Если поставить себе в цель реализацию потокобезопасной системы событий подобным образом — все можно сделать очень грамотно.

                              «некуда засунуть» — это уже детали конкретной реализации.
                            • +2
                              Общий ресурс — это уже другая проблема. Для наблюдаемого объекта нет опасности получить NRE и ObjectDisposedException. А это именно та проблема, которая описывается в статье.
                        • 0
                          DEL — повтор мысли в сообщении выше
            • 0
              Пример приводился в статье. Вызвали отписавшийся обработчик — получили ObjectDisposedException
              • –3
                Тогда почему не рассматривается возможность получения ArgumentException? Или IOException? Нет, серьезно, почему мы не предусматриваем возможность поломки жесткого диска в процессе обработки нашего события, раз уж нам так важно обезопасить себя, не имея возможности влиять на чужой код?

                Если важно, чтобы из-за упавшего обратного вызова наш код не поломался — надо использовать try… catch или try… finally. Эта рекомендация является общей для любых вызовов внешнего кода — и проблема отписки от события тут ни при чем.
                • +1
                  Тогда почему не рассматривается возможность получения ArgumentException? Или IOException?

                  Потому что логично предположить, что в таких местах обработчик сам ловит эти исключения — это его прямая обязанность, которая документирована и является общепринятой. Случай с try / catch я тоже рассматривал — в этом случае вызов обработчиков прервется при первом исключении.

                  Всё, что я хотел сказать этой статьёй — проверка на null не обеспечит полную потокобезопасность вашего кода (как это утверждается в примерах и книгах, в частности в CLR via C#), и это нужно иметь в виду.
                  • 0
                    Что вы понимаете под потокобезопасностью? И почему обработчик не может сам поймать ObjectDisposedException — ведь это тоже, вообще-то, его прямая обязанность?
                    • 0
                      Под потокобезопасностью я понимаю ту самую цитату, которую я привел в самом конце:
                      Implementation is guaranteed to be free of race conditions when accessed by multiple threads simultaneously.


                      Само использование делегатов вносит состояние гонки, поэтому, грубо говоря, любое использование делегатов в многопоточном окружении не является потокобезопасным. Это можно решить «прививая» процесс недоверия событию на уровне документации и общепринятых практик.

                      Насчет «отлова» ObjectDisposedException я написал в статье — с точки зрения разработчика вполне логично думать, что у нас никогда не возникнет ситуации, в которой вызовется обработчик освобожденного объекта, и можно просто не городить ненужные по нашему мнению блоки try / catch. Случай, когда обработчик сам ловит свой ObjectDisposedException полностью аналогичен проверке на if(IsDisposed == true).

                      Эти «баги» с вызовом отписавшихся событий крайне сложно ловить, если не знать о чем речь и в чем причина.
                      • 0
                        Implementation is guaranteed to be free of race conditions when accessed by multiple threads simultaneously.
                        «Free of race conditions» — слишком расплывчатое определение. Скажем, с моей точки зрения тут никаких гонок внутри объекта нет.

                        Само использование делегатов вносит состояние гонки
                        Как?!
                        • 0
                          Я же писал про это (в частности цитировал и Джона Скита, см. источники в конце статьи) — после начала вызова события (UPD: но до его окончания) у других потоков есть возможность отписаться от него (не имеет значения — копировали ли мы делегат в локальную переменную или нет). Поскольку исход подобного действия зависит от количества работающих с объектом потоков — это является состоянием гонки.
                          • 0
                            Поскольку исход подобного действия зависит от количества работающих с объектом потоков — это является состоянием гонки.
                            Как раз-таки, если скопировать делегат в локальную переменную, то в результате будут вызваны все подписчики, существовавшие на момент появления события (точнее, на момент копирования) независимо от количества потоков.
                            • 0
                              Даже если не копировать — будет то же самое (об этом я тоже писал -после начала вызова делегат уже не будет меняться). От количества потоков в данном случае зависит поведение самих подписчиков. Зависимость никуда не девается.
                              • 0
                                Почему, определяя наличие гонок в нашем коде, нас интересует поведение подписчиков?
                                • 0
                                  Вы серьезно? Потому что мы можем получить, в частности, ObjectDisposedException, поскольку мы не можем доверять обработчикам, которые не знают, что могут быть вызваны после отписки от события. Это приведет к неопределенному поведению нашей системы события в целом. В идеале обработчики либо не должны влиять на процесс вызова события, либо выполнять контракт, согласно которому они обязуются не обрабатывать события, от которых уже отписались. Поскольку ни то, ни другое у нас не обеспечивается — частичная ответственность за безопасность вызова лежит на нас.
                                • 0
                                  Таким же точно образом можно сказать, что обработчик не должен вызываться после отписки метода. Почему нас должна волновать причина, по которой вызывается обработчик, который уже отписался от события? Я отписался — я не хочу больше получать события — я ничего не знаю.
                                  • 0
                                    Почему нас должна волновать причина, по которой вызывается обработчик, который уже отписался от события?
                                    Потому что вызов обработчика, отписанного от события, ничем не отличается от отписывания от события обработчика, который уже выполняется (надо просто представить, что у него сейчас выполняется нулевая строчка).

                                    Любой способ решения ситуации номер 2 автоматически решит и ситуацию номер 1 — а потому бороться с ситуацией номер 1 отдельно не имеет смысла.
                    • 0
                      А вообще мне кажется я понял, почему Рихтер уделил этому вопросу так мало внимания — у вас вполне может быть одинаковый ход мыслей. Для него, как и для вас, этот вопрос просто не имеет большого веса, и главная в данном случае задача — не допустить возникновения NullReferenceException с «нашей» стороны.

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

      void foo()
      {
      a = true;
      b = true;
      }

      void foo2()
      {
      if (b)
      {
      // тут a может быть false
      }
      }
      • 0
        Это вы вообще к чему? В каком из моих примеров вы нашли аж две переменные? :)
        • 0
          Вы отписываетесь от события а потом ставите флаг. Может быть ситуация, когда Вы отписались от события, но флаг еще равен false. Компилятор и процессор не в курсе что переменная делегата и вашего флага зависимые между потоками.
          Компилятор и процессор могут менять порядок выполнения в целях оптимизации, может кешировать в локальном треде. Получается Ваш пример тоже не тред сейв. С Уважением.
          • 0
            Пожалуйста, прочитайте мой комментарий внимательнее.
            • 0
              Прочитал. Пока писал ответ нашел Ваш же комментарий где флаг обернут в lock, вот собственно такой вариант решает проблему которую я описал.

              Мы может после флага изменить еще что нибудь и в обработчике это что нибудь будет изменено а флаг нет. А lock ставит барьеры и такого не произойдет. С уважением.

              • 0
                Значит, вы так ничего и не прочитали. Я приводил не код, который решает проблему — а код, который ее демонстрирует. Флаг — это не решение проблемы, а пример общего ресурса, при доступе к которому возникает гонка.
  • +4
    Спасибо за расстановку точек над i, у меня в голове сильно прояснилось.

    Рихтера читал и верил мэтру на слово, пустой делегат считал идеальным лобовым решением на случай, если действительно будет жесткая конкуренция на событии (с чем пока не столкнулся :), а тут вон оно как. :)
    • +2
      Чисто технически, пустой делегат почти ничем не отличается от метода Рихтера с Volatile.Read, оверхед в 5 наносекунд в большинстве приложений не заметен. Есть однозначный момент, когда стоит использовать проверку на null — в случае, если создание объекта EventArgs занимает много времени и памяти.
  • +1
    совершенно очевидно, что вызов уже отписанного обработчика надёжно предотвратить может только сам обработчик. ведь отписка может конкурентно произойти как раз в момент собственно вызова. таким образом, в источнике событий нам остаётся только проверка на пустой/null делегат
    • 0
      Разумеется, об этом я написал в итогах.
  • +2
    Мне вообще не нравится практика использования событий в многопоточной среде.
    У вас 2 класса работают в 2 потоках. Первый подписывается на второй, второй вызывает событие и вуаля: код первого класса вызывается из второго потока и вам приходится писать внутренности первого класса потокобезопасно. Просто прекрасно.
    Мне больше импонирует модель акторов (с посылкой асинхронных сообщений из одного потока в другие). Правда тогда приходится писать циклы обработки.
    • 0
      Да, я думал об общей смене парадигмы этого шаблона Observer, но к сожалению, для потокобезопасной системы событий в C# видимо придется отойти от внутренней «поддержки» этой системы — делегатов и событий (event). Тут проблема в самих делегатах, от этого никак не убежать.
      • 0
        А вот если dispose'ить всякие data access layer, service layer и прочие, инициируя из наблюдаемого объекта (предположим, что приложение спроектировано без перекрёстного наблюдения и прочих кривых решений) с потокобезопасным завершением всех действий. Т. е. начали dispose'ить, после этого новые действия, в т. ч. генерирование событий, не начинаем, но ждём завершения уже начатых. При этом наблюдаемый объект остальные (вроде service layer) создаёт и удаляет, для остального возможны события и прочие callback'и. Но если завершить все вызовы перед тем, как начать удалять всякие наблюдающие объекты, то и событию будет вызваться неоткуда. Т. е. если, например, новое remote-уведомление поступит в момент dispose, наблюдаемый объект при его обработке вернёт обрабатываемую ошибку, что он уже начал dispose'иться и новых вызовов не принимает. В случае разработки toolkit'а для стороннего использования это, конечно, не реализовать (да и вообще, по-моему, сложно защитить от кривого использования), но для end user-проектов вроде как решение. Поправьте, если где ошибаюсь.
        • 0
          Можно поступить еще проще — просто не забывать о проблеме. Не так и трудно переписать код, использующий события, чтобы он стал потокобезопасным.
  • +3
    Побольше бы таких статей. Очень понравился стиль изложения(или перевод), очень легко читается. Спасибо автору.
    Кстати, может я и ошибаюсь, но после «большого раздела» минусовать стали намного меньше…
  • +12
    Мне кажется, что решением проблемы может быть замена событий на Rx.
    Возможностей больше, управление легче да и стандартными функциями гарантируется соблюдение контракта, что отписанный метод не будет вызван.
    • +1
      Отлично, я ждал подобных комментариев, спасибо
    • 0
      Тоже, открывая статью, был уверен что напишут про Rx. Ещё как вариант — event aggregator :-).
      PS: сам всегда использую вариант с = delegate {}.
    • 0
      Полностью согласен. Вспомнился старый анекдот. Перефразируя:

      1. Для data-oriented приложений всегда необходимо использовать Rx.
      2. Пишем data-oriented приложения на событиях (.net events). См. пункт первый :)
  • 0
    А если попробовать сделать обертку над подписчиками, а в вызове обертки вставлять проверку на отписку?
    public event EventHandler Event
    {
    add { event += Wrapper.for(value); }
    remove { event -= Wrapper.for(value); }
    }
    Как-то так?
    • +1
      Проблема в том, что для разработчика процесс вызова делегата выглядит атомарным, мы не можем проверять и вызывать подписчики по одному.
      Тут проблема не столько в том, что копирование в локальную переменную позволяет шустрым подписчикам отписаться между локальной «заморозкой» и вызовом события, сколько в том, что при отписке обработчиков уже во время вызова на сам вызов это никак не повлияет —
      нет никакой возможности проверить, отписался ли кто-то от события во время вызова или нет

      Единственным способом отойти от подобной проблемы — использовать другую систему событий, как писали выше.
      • +1
        Проблема в том, что для разработчика процесс вызова делегата выглядит атомарным, мы не можем проверять и вызывать подписчики по одному.
        Забавно, но именно это мы как раз и можем сделать :)

        Я это использовал в одном из проектов, когда понадобилось уведомлять подписчиков параллельно, а не последовательно. Другое дело, что проблему с отпиской это никак не решает (потому что проблема не в отписке, как я уже говорил)
        • +1
          Пропустил этот момент, спасибо за замечание.

          Этот метод с проверкой мне представляется крайне медленным — придется перед вызовом очередного обработчика получать новый список, брать первый попавшийся делегат, сравнивать со списком тех, что мы уже вызвали и т.д.
          • 0
            Простите, но какой «метод с проверкой» является крайне медленным? Который все равно не работает? :)
            • 0
              Ну да, об этом я тоже написал в статье
              Технически, это может спасти от вызова отписавшихся обработчиков, но мы не можем быть уверенными в том, какие действия сделал объект-подписчик перед тем, как отписаться от события, поэтому мы все еще можем напороться на «испорченный» объект


              Про медленность метода я отметил как еще один недостаток подобного подхода, но вы меня поймали, каюсь
            • 0
              А если при отписке в Wrapper выставлять флаг, а внутри вызова проверять его и если он выставлен не вызывать нижележащий делегат? Wrapper всегда один и тот же и не страшно, что кто-то его уже захватил через GetInvocationList
              • 0
                Проблема не в вызове отписанного обработчика, проблема в том, что обработчик может быть отписан, когда он уже выполняется. Проверка флага тут не спасет.
                • 0
                  Как мне кажется это уже надуманная проблема, если метод выполняется, то уже неважно отписан обработчик или нет. Иначе по-вашему получается что нужно проверять состояние объекта на каждой строке. Если внутри выполняется что-то совсем критичное, то нужно делать блокировку на это время, и об этом уже лучше знать разработчику такого объекта.
                  • 0
                    Проблема «вызван отписанный обработчик» — это частный случай проблемы «обработчик отписан в процессе выполнения». Если вторая проблема, как вы говорите, надумана — то зачем бороться с ее частным случаем?
                    • 0
                      Если обработчик отписан в процессе выполнения, то вызов метода уже идет и возможно стоит заблокировать саму возможность отписаться в этот момент и/или возможность уничтожить такой объект до окончания вызова (как вариант маркировать объект как поставленный на отписку, чтобы не заблокировать полностью возможность отписаться). В случае блокировки проблема отпадает.
                      В первом случае обработчик уже прошел процедуру отписки и это мы неправильно его захватили, он может быть в невалидном состоянии и имеет право развалиться на пол пути, так что мне кажется это два разных случая.
                      • +1
                        Если в двух случаях программа одинаково падает (одно и то же исключение вываливается в одной и той же строчке с одним и тем же стэком вызовов) — то почему в одном случае она имеет право так сделать — а во втором это мы виноваты?
                • +1
                  Нужно запретить вызывать событие напрямую, а только через обёртку.
                  Обёртка должна создавать event в состоянии non-signaled, записывать его куда в static-поле, вызывать цепочку делегатов, затем удалять event.

                  При отписывании метод remove должен взять event из static-поля, удалить делегата из цепочки, сделать Wait на event-е.

                  Разбавить всё критическими секциями по вкусу.

                  Таким образом, если в момент входа в remove выполнялись делегаты, выход из remove будет ждать, пока все делегаты, которые стартовали на момент входа в remove, не завершаться. Это гарантирует, что после выхода из remove отписавшийся объект не будет вызван и можно удалять ресурсы.
                  • 0
                    Да, я тоже в итоге пришел к выводу, что надо ждать завершения обработчика в методе remove. Вот только про static-поле я как-то не понял — а что, если одновременно случится несколько разных событий? Или даже одно и то же событие несколько раз?

                    Кроме того, тут есть проблема самоотписывания обработчика. Ваше решение в таком случае попросту повиснет (ожидание на событии, которое может установить только тот же самый поток).

                    Ну и, наконец, мне не нравится идея, что для того, чтобы отписать один обработчик, надо дождаться всех остальных. Все-таки ожидание на отписывании от события — вещь неожиданная, и надо это ожидание свети к минимуму.
                    • 0
                      Да, я тоже в итоге пришел к выводу, что надо ждать завершения обработчика в методе remove. Вот только про static-поле я как-то не понял — а что, если одновременно случится несколько разных событий? Или даже одно и то же событие несколько раз?

                      Тут главное, чтобы remove взял event, созданный не позже, чем fireEvent (так назову «обёртку» для краткости) взял цепочку делегатов на вызов. static-поле и цепочку даже можно защитить одной критической секцией.
                      Разумеется, fireEvent только пишет в static-поле, а на стеке у него копия event-а, чтобы удалить ту копию, которую он и создал.

                      Кроме того, тут есть проблема самоотписывания обработчика. Ваше решение в таком случае попросту повиснет (ожидание на событии, которое может установить только тот же самый поток).

                      Значит, надо в static-поле класть ссылку на пару: event + ThreadId. Если ThreadId в remove совпадает с сохранённым в fireEvent, то не ждать. Хотя надо ещё подумать, возможны ли сценарии взаимоблокировки между 2-3 потоками.

                      Ну и, наконец, мне не нравится идея, что для того, чтобы отписать один обработчик, надо дождаться всех остальных. Все-таки ожидание на отписывании от события — вещь неожиданная, и надо это ожидание свети к минимуму.

                      Ничего не поделать, это условие задачи.
                      • 0
                        Ничего не поделать, это условие задачи.
                        С чего бы это вдруг? Я же нашел, что тут можно поделать!
    • 0
      Против ситуации номер 2 (см. мой комментарий) — не поможет. А значит — бесполезно.
  • +1
    Не смотря на то, что я по-прежнему считаю, что простой вызов события через локальную переменную является потокобезопасным — я сделал еще более защищенную реализацию.

    Вкратце о том, как она работает:
    1. Проблема, о которой говорится в статье, не в том, что обработчик вызывается после отписки от события — а в том, что его можно отписать от события в то время, пока он выполняется. Решаю я ее просто: функция отписки от события ждет, пока завершится выполнение обработчика.
    2. Тут вылезает вторая проблема: обработчик может попытаться отписать сам себя. Разумеется, надо позволить ему сделать это без вечной блокировки. Для этого я запоминаю номер потока, в котором выполняются обработчики событий.
    3. Ну и для того, чтобы не возникало проблем с одновременным выполнением одного и того же события в разных потоках, я создаю на каждую операцию обработки события по отдельному объекту.
    4. Для того, чтобы было удобнее использовать мой код, я его оформил в отдельную структуру, управляющую подпиской/отпиской/обработкой события.

    Вот, собственно, сам код
        public struct EventHelper
        {
            private event Action handlers;
            private event Action<Action> removeHooks;
    
            public event Action Handlers
            {
                add
                {
                    if (value.GetInvocationList().Length > 1)
                        throw new ArgumentException("Event handler must be single method");
                    handlers += value;
                }
    
                remove
                {
                    if (value.GetInvocationList().Length > 1)
                        throw new ArgumentException("Event handler must be single method");
                    handlers -= value;
    
                    var r = removeHooks;
                    if (r != null) r(value);
                }
            }
    
            public void Invoke()
            {
                var h = handlers;
                if (h == null) return;
    
                var i = new Invokation(h.GetInvocationList());
    
                removeHooks += i.RemoveHandler;
                try
                {
                    i.Invoke();
                }
                finally
                {
                    removeHooks -= i.RemoveHandler;
                }
            }
    
            private class Invokation
            {
                private readonly Delegate[] handlers;
                private Delegate currentHandler;
                private Delegate waiting;
                private Thread ownerThread;
    
                public Invokation(Delegate[] handlers)
                {
                    this.handlers = handlers;
                }
    
                public void Invoke()
                {
                    this.ownerThread = Thread.CurrentThread;
    
                    for (var i = 0; i < handlers.Length; i++)
                    {
                        Action handler;
                        lock (this)
                        {
                            currentHandler = handler = (Action)handlers[i];
                            waiting = null;
                        }
    
                        if (handler != null)
                            try
                            {
                                handler();
                            }
                            finally
                            {
                                currentHandler = null;
                                lock (this)
                                    if (waiting == handler)
                                        Monitor.Pulse(this);
                            }
                    }
                }
    
                public void RemoveHandler(Action handler)
                {
                    lock (this)
                    {
                        if (Thread.CurrentThread != ownerThread && handler == currentHandler)
                        {
                            waiting = handler;
                            Monitor.Wait(this);
                        }
    
                        var index = Array.IndexOf(handlers, handler);
                        if (index != -1)
                            handlers[index] = null;
                    }
                }
            }
        }
    

    Говорю сразу: я его даже ни разу не запускал, потому что некогда. Но на выходных постараюсь довести его до ума.
    • 0
      UPD: да, совсем забыл, после завершения Invokation.Invoke надо обнулить ownerThread
    • 0
      Мне тоже показалось, что потокобезопасность тут проблема потребителя событий, а не источника. Если он в одном потоке сначала отписывается от события, а потом начинает диспозить зависимости, то в другом потоке источник может бросить событие и раньше, чем произошла отписка, то есть у источника рейс-кондишен не повлиял, на момент бросания события потребитель был еще подписан, но в процессе обработки в двух разных потоках у потребителя наперегонки в одном вызываются методы на зависимостях, а в другом зависимоти уничтожаются.

      Кстати насчет оператора lock вопрос — разве он сам не учитывает, что уже существующая блокировка принадлежит этому же потоку и надо просто проигнорировать? Или это надо попросить у микрософта включить в С# 6 в разделе новых фишек.
      • 0
        Lock-то проверяет. Но я же использую Monitor.Wait.
    • 0
      removeHooks не защищён критической секцией.
      При параллельном вызове Invoke() из разных потоков может какой-то RemoveHandler либо не добавиться, либо при выходе не удалиться
      • 0
        С чего бы? Это же автоматический event — а им компилятор сам защиту делает.
        • 0
          Декомпилировал — был удивлён. Причём используется неблокирующая реализация (Interlocked.CompareExchange)
          • 0
            Я это отмечал пару раз в статье кстати.
  • 0
    Для многопоточного программирования это нормальная ситуация, потоки действуют в произвольном порядке, в произвольные моменты времени. Если вы хотите каких то гарантий — используйте соотв. методы: синхронизацию, атомарные операции, локи или лок-фри алгоритмы. Может быть не использовать стандартные события вообще, а реализовать собственные? Тогда будет полный контроль над происходящим и не нужно будет гадать.
  • +1
    Очень интересно было бы прочитать про примеры таких приложений, где «много потоков постоянно подписываются и отписываются от какого-то события».
    Интересно, использует ли кто-то реально события в таких приложениях или все-таки другие методы (очереди сообщений, например)?
    • 0
      Я работал над проектом, где именно так все и происходило. Если интересно, что это было — то это была библиотека для работы с сетевым сканером отпечатков пальцев. Но если бы я начал тот проект с нуля — я бы использовал словарь обработчиков вместо постоянной подписки/отписки, примерно как сделано в статье UDP и C# async/await
    • 0
      Например, клиент p2p-сетей. Каждый пир — отдельный поток, чтобы легче было программировать протокол. Пиры часто появляются и пропадают. Какие-нибудь UI-Manager, DHT-Manager, IO-Manager подписываются на события от пиров, пиры подписываются на системные события.
  • 0
    После столь детального исследования вопроса работодатель, случайно, не попросил Вас взять его в ученики?
    • 0
      Я всё ещё ищу работу.
      Даже на джуниора почему-то брать не хотят.
      Такие дела.

      (хокку соискателя)
  • 0
    >>Всё становится совершенно запутанным — оба варианты равнозначны, но тот, что с Volatile.Read более равнозначен

    Ничего запутанного. Кроме CLR существует еще и Mono (которая использует ECMA Memory model, т.е. CLR 1). Именно для этих целей используется Volatile.Read, чтобы гарантировать корректную работу, например, на Itanium-машинах.

    CLR начиная с версии 2.0 предоставляет новую модель памяти и Volatile.Read можно опустить.
  • +1
    Вообще, для ознакомления с историей событий в .NET рекомендую серию статей от Chris Borrows.

    Касательно темы потокобезопасности событий не рассматривается вопрос по reliability: что если делегат не подготовлен (см. метод RuntimeHelpers.PrepareContractedDelegate и статью Keep Your Code Running with the Reliability Features of the .NET Framework?

    private static readonly object _exceptionLock = new object();
    private static EventHandler<EventArgs> _someEvent = delegate { };
    
    public static event EventHandler<EventArgs> SomeEvent 
    {
        add
        {
            if (value == null)
                return;
            RuntimeHelpers.PrepareContractedDelegate(value);
            lock (_exceptionLock)
                _someEvent += value;
        }
        remove
        {
            lock (_exceptionLock)
                _someEvent -= value;
        }
    }
    


    В вышеприведенном примере получаем и потокобезопасность, и «защиту» от NullReferenceException, и reliability (вспомним про события вроде DomainUnload, ProcessExit, and UnhandledException).

    А теперь рассмотрим Constrained Execution Regions.
    Что если событие вызывается из под CER (см. статью Event Accessors в колонке .NET Matters)? Т.е. подписавшийся делегат также должен быть подготовлен заранее.

    Просто замените строчку с
    RuntimeHelpers.PrepareContractedDelegate(value);
    

    на
    RuntimeHelpers.PrepareDelegate(value);
    

    Если проверка на null поля с делегатом уже карго-культ, то гипотетический вызов уже отписавшихся подписчиков тем более.
  • 0
    Мне понравился вариант, описанный тут:
    static void LockFreeUpdate<T> (ref T field, Func <T, T> updateFunction)
      where T : class
    {
      var spinWait = new SpinWait();
      while (true)
      {
        T snapshot1 = field;
        T calc = updateFunction (snapshot1);
        T snapshot2 = Interlocked.CompareExchange (ref field, calc, snapshot1);
        if (snapshot1 == snapshot2) return;
        spinWait.SpinOnce();
      }
    }
    

    Here’s how we can use this method to write a thread-safe event without locks (this is, in fact, what the C# 4.0 compiler now does by default with events):

    EventHandler _someDelegate;
    public event EventHandler SomeEvent
    {
      add    { LockFreeUpdate (ref _someDelegate, d => d + value); }
      remove { LockFreeUpdate (ref _someDelegate, d => d - value); }
    }
    
    • +2
      Вы не читали статью, и даже не поняли, о чем вообще был флуд в комментариях выше…
      • 0
        Извиняюсь, промахнулся — это было к комментарию выше. А точнее к тому, что можно обойтись без lock.
        • 0
          Пожалуйста, расскажите где именно я писал, что можно обойтись без lock.

          p.s.
          статью читал, комментарии выше также.
        • +1
          сорри, неправильно понял посыл коммента ;)
  • 0
    Чего-то я не пойму. Проблема в том, что после вызова Delegate.Invoke уже нельзя изменить Invocation List по мере обхода подписчиков делегата. Ну так и не засовывайте всё в кучу, если нужен контроль.

    public sealed class SafeEventInvoker
    {
        private HashSet<EventHandler> m_Subscribers = new HashSet<EventHandler>();
    
        protected void RaiseEvent()
        {
            // Локальная копия требуется по тому, что при использовании итератора 
            // объект будет заблокирован до завершения вызова всех подписок.
    
            EventHandler[] subscribersCopy;
            lock (m_Subscribers)
                subscribersCopy = m_Subscribers.ToArray();
    
            // Блокировка на каждый отдельный вызов. При этом если из 
            // делегата будет вызвана отписка от события дедлока не произойдет т.к. 
            // Monitor.Enter реентерабелен в контексте потока.
    
            foreach (var subscriber in subscribersCopy)
                lock (m_Subscribers)
                    if (m_Subscribers.Contains(subscriber))
                        subscriber(this, EventArgs.Empty);
        }
    
        public event EventHandler ThreadSafeEvent
        {
            add
            {
                if (null != value)
                    lock (m_Subscribers)
                        m_Subscribers.Add(value);
            }
            remove
            {
                if (null != value)
                    lock (m_Subscribers)
                        m_Subscribers.Remove(value);
            }
        }
    }
    • +1
      Нет, проблема не вполне в этом.
      • +1
        Аргументируйте. Кроме минусов.
        • 0
          habrahabr.ru/post/240385/#comment_8071457
          и еще вот эта ветка: habrahabr.ru/post/240385/#comment_8073981
          Извиняюсь, но я уже устал повторять одни и те же аргументы разным слушателям.
          • 0
            Правильно. Если немного вчитаетесь, то поймете, что при использовании кода, который я привёл, операция
            SafeEventInvoker.ThreadSafeEvent -= Handler;

            не сможет вернуть управление, пока не завершится вызов Handler, если он был прерван в процессе выполнения потока.
            Всё равно проблема вызова после отписки и отписки в процессе выполнения сводится к атомарности вызова делегата.
            Само собой, приведенный код не эталон производительности. Но на вскидку — решающий проблемы гонок, блокировок и отписок.
            • +1
              Тогда ваш код не соответствует вашему же описанию…
              • 0
                Эммм…
                • Проблема в том, что после вызова Delegate.Invoke уже нельзя изменить Invocation List по мере обхода подписчиков делегата.
                • Всё равно проблема вызова после отписки и отписки в процессе выполнения сводится к атомарности вызова делегата. Само собой, приведенный код не эталон производительности. Но на вскидку — решающий проблемы гонок, блокировок и отписок.
                ?
                • +1
                  Проблема не в том, что после вызова Delegate.Invoke уже нельзя изменить Invocation List по мере обхода подписчиков делегата. Есть куча решений, позволяющих менять этот список — но проблема остается исключительно в том делегате, который выполняется прямо сейчас — и который нельзя прервать.
    • +1
      Вот, не прошло и года, реализация еще более безопасных событий с использованием врапперов.
          public struct Event<T>
          {
              private Wrapper[] wrappers;
      
              public void Subscribe(T subscriber)
              {
                  var w = new Wrapper { subscriber = subscriber };
                  Wrapper[] old;
                  do { old = wrappers; } while (Interlocked.CompareExchange(ref wrappers, Add(old, w), old) != old);
              }
      
              public void Unsubscribe(T subscriber)
              {
                  Wrapper w = null;
                  Wrapper[] old = wrappers;
                  for (var i = 0; i < old.Length; i++)
                      if (old[i] != null && object.Equals(old[i].subscriber, subscriber))
                          w = old[i];
                  if (w == null) return;
                  do { old = wrappers; } while (Interlocked.CompareExchange(ref wrappers, Del(old, w), old) != old);
                  w.Close();
              }
      
              public void Invoke(Action<T> runner)
              {
                  var old = wrappers;
                  for (var i = 0; i < old.Length; i++)
                  {
                      var w = old[i];
                      if (w != null && w.StartRun())
                          try { runner(w.subscriber); }
                          finally { w.FinishRun(); }
                  }
              }
      
              public void Invoke(Action<T, Action> runner)
              {
                  var old = wrappers;
                  for (var i = 0; i < old.Length; i++)
                  {
                      var w = old[i];
                      if (w != null && w.StartRun())
                          runner(w.subscriber, w.FinishRun);
                  }
              }
      
              public void Invoke(Action<Action<Action<T>>> dispather)
              {
                  var old = wrappers;
                  for (var i = 0; i < old.Length; i++)
                  {
                      var w = old[i];
                      if (w != null && w.StartRun())
                          dispather(runner =>
                          {
                              try { runner(w.subscriber); }
                              finally { w.FinishRun(); }
                          });
                  }
              }
      
              private static Wrapper[] Add(Wrapper[] array, Wrapper value)
              {
                  if (array == null) return new[] { value, null, null, null };
      
                  for (var i = 0; i < array.Length; i++)
                      if (array[i] == null && Interlocked.CompareExchange(ref array[i], value, null) == null)
                          return array;
      
                  var oldlen = array.Length;
                  Array.Resize(ref array, oldlen + (oldlen + 1) / 2);
                  array[oldlen] = value;
                  return array;
              }
      
              private static Wrapper[] Del(Wrapper[] array, Wrapper value)
              {
                  if (array == null) return null;
      
                  for (var i = 0; i < array.Length; i++)
                      if (array[i] == value)
                          array[i] = null;
      
                  return array;
              }
      
              private class Wrapper
              {
                  public T subscriber;
                  public int state = 0;
      
                  public bool StartRun()
                  {
                      int old;
                      do { old = state; } while (old >= 0 && Interlocked.CompareExchange(ref state, old + 1, old) != old);
                      return old >= 0;
                  }
      
                  public void FinishRun()
                  {
                      int old;
                      do { old = state; } while (Interlocked.CompareExchange(ref state, old >= 0 ? old - 1 : old + 1, old) != old);
                      if (old < -1) lock (this) Monitor.PulseAll(this);
                  }
      
                  public void Close()
                  {
                      int old;
                      do { old = state; } while (old >= 0 && Interlocked.CompareExchange(ref state, ~old, old) != old);
      
                      if (old == -1 || old == 0) return;
                      lock (this) if (state < -1) Monitor.Wait(this);
                  }
              }
          }
      


      Идея все та же: не давать отписать обработчик от события в то время как он выполняется.
      • 0
        UPD: извиняюсь, не туда написал

Только зарегистрированные пользователи могут оставлять комментарии. Войдите, пожалуйста.