Как стать автором
Обновить

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

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

Рассмотрим такой код:
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();
}
Уж параметр метода-то ни при каких оптимизациях измениться между проверкой и вызовом не должен…
Уж параметр метода-то ни при каких оптимизациях измениться между проверкой и вызовом не должен…
А ещё JIT умеет инлайнить методы
А можно использовать
[MethodImpl(MethodImplOptions.NoInlining)]
В целом своим комментарием просто соглашаетесь с моим итогом:
А следовательно, защита от вызова события после отписывания от него находится в области ответственности внешнего кода.

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

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

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

А поскольку мы не можем быть уверенными в потокобезопасной реализации обработчиков, общее решение также не является потокобезопасным.
Вы заменили гонку из статьи собственно придуманной гонкой, но суть проблемы это не меняет. Опять же можно в вашем варианте попробовать залокировать отписку и операции с переменной done, но и здесь, аналогично, как в статье, всплывает проблема deadlock. Чисто техническая замена внутренней проверки на проверку извне ничего не даст.
Почему же не даст?
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;
    }
}


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

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

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

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

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

А разделяемые ресурсы тут при том, что в отсутствие разделяемых ресурсов никаких гонок не возникает:
foo.SomeEvent += SomeHandler;
...
foo.SomeEvent -= SomeHandler;

//

void SomeHandler() {
    Console.WriteLine("Hello, world!");
}

К примеру, в этом коде нам совершенно не важно, будет или нет вызван метод SomeHandler в случае одновременной отписки от события и его возникновения.
Я не совсем к этому, я к тому, что не надо включать разделяемые ресурсы в Lock данном случае. Достаточно включить флаг и потом его в локе проверить. Зачем критическую секцию кидать на все?
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(); вне критической секции.
Я о нижнем локе, а не локе в экшене. Плюс преепишите lock(_lock) if(done) return; res.Do...(); и вот вам Do без критической секции?
Если сделать любую из предложенных вами правок — возникнет возможность выхода из блока using в то время, когда в другом потоке выполняется res.DoSomething()
Да, тут я с вами полностью согласен.
А следовательно, защита от вызова события после отписывания от него находится в области ответственности внешнего кода.

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

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

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

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

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

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

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

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?
Так у вас всё та же подписка на события, коллбэки и прочее. Это ни чем концептуально не отличается от механизма event-ов, о которых говорится в статье. А выше предлагают другой механизм, без коллбэков. Где push-модель заменяется на pull (подписчик сам берёт сообщение из очереди, если оно ему нужно). Получается, что у наблюдаемого объекта нет коллекции колбэков, а это значит, что никто обработчик события не вызовет, если наблюдатель этого уже не хочет.
Вы правда думаете, что в pull-модели некуда засунуть общий ресурс, при доступе к которому произойдет ошибка?

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

Вкратце о том, как она работает:
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;
                }
            }
        }
    }

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

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

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

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

CLR начиная с версии 2.0 предоставляет новую модель памяти и Volatile.Read можно опустить.
Вообще, для ознакомления с историей событий в .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 поля с делегатом уже карго-культ, то гипотетический вызов уже отписавшихся подписчиков тем более.
Мне понравился вариант, описанный тут:
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); }
}
Вы не читали статью, и даже не поняли, о чем вообще был флуд в комментариях выше…
Извиняюсь, промахнулся — это было к комментарию выше. А точнее к тому, что можно обойтись без lock.
Пожалуйста, расскажите где именно я писал, что можно обойтись без lock.

p.s.
статью читал, комментарии выше также.
сорри, неправильно понял посыл коммента ;)
Чего-то я не пойму. Проблема в том, что после вызова 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);
        }
    }
}
Нет, проблема не вполне в этом.
Аргументируйте. Кроме минусов.
Правильно. Если немного вчитаетесь, то поймете, что при использовании кода, который я привёл, операция
SafeEventInvoker.ThreadSafeEvent -= Handler;

не сможет вернуть управление, пока не завершится вызов Handler, если он был прерван в процессе выполнения потока.
Всё равно проблема вызова после отписки и отписки в процессе выполнения сводится к атомарности вызова делегата.
Само собой, приведенный код не эталон производительности. Но на вскидку — решающий проблемы гонок, блокировок и отписок.
Эммм…
  • Проблема в том, что после вызова Delegate.Invoke уже нельзя изменить Invocation List по мере обхода подписчиков делегата.
  • Всё равно проблема вызова после отписки и отписки в процессе выполнения сводится к атомарности вызова делегата. Само собой, приведенный код не эталон производительности. Но на вскидку — решающий проблемы гонок, блокировок и отписок.
?
Проблема не в том, что после вызова Delegate.Invoke уже нельзя изменить Invocation List по мере обхода подписчиков делегата. Есть куча решений, позволяющих менять этот список — но проблема остается исключительно в том делегате, который выполняется прямо сейчас — и который нельзя прервать.
Вот, не прошло и года, реализация еще более безопасных событий с использованием врапперов.
    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);
            }
        }
    }


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

Публикации

Истории