Это действительный шаблон для поднятия событий в C #? - PullRequest
Новая
       25

Это действительный шаблон для поднятия событий в C #?

10 голосов
/ 23 января 2010

Обновление : Для всех, кто читает это, начиная с .NET 4, блокировка не нужна из-за изменений в синхронизации автоматически генерируемых событий, поэтому я просто использую это сейчас: 

public static void Raise<T>(this EventHandler<T> handler, object sender, T e) where T : EventArgs
{
    if (handler != null)
    {
        handler(sender, e);
    }
}

И чтобы поднять его: 

SomeEvent.Raise(this, new FooEventArgs());
<ч />

Прочитав одну из статей Джона Скита о многопоточности , я попытался инкапсулировать подход, который он поддерживает, для вызова события в методе расширения, например, (с аналогичной общей версией): 

public static void Raise(this EventHandler handler, object @lock, object sender, EventArgs e)
{
    EventHandler handlerCopy;
    lock (@lock)
    {
        handlerCopy = handler;
    }

    if (handlerCopy != null)
    {
        handlerCopy(sender, e);
    }
}

Это можно тогда назвать так: 

protected virtual void OnSomeEvent(EventArgs e)
{
    this.someEvent.Raise(this.eventLock, this, e);
}

Есть ли проблемы с этим?

Кроме того, меня немного смущает необходимость блокировки в первую очередь. Насколько я понимаю, делегат скопирован в примере в статье, чтобы избежать возможности его изменения (и обнуления) между нулевой проверкой и вызовом делегата. Однако у меня сложилось впечатление, что такого рода доступ / назначение являются атомарными, так зачем нужна блокировка?

Обновление: Что касается комментария Марка Симпсона ниже, я собрал тест: 

static class Program
{
    private static Action foo;
    private static Action bar;
    private static Action test;

    static void Main(string[] args)
    {
        foo = () => Console.WriteLine("Foo");
        bar = () => Console.WriteLine("Bar");

        test += foo;
        test += bar;

        test.Test();

        Console.ReadKey(true);
    }

    public static void Test(this Action action)
    {
        action();

        test -= foo;
        Console.WriteLine();

        action();
    }
}

Это выводит: 

Foo
Bar

Foo
Bar

Это показывает, что параметр делегата для метода (action) не отражает аргумент, который был передан в него (test), что, как я полагаю, является ожидаемым. Мой вопрос, это повлияет на действительность блокировки в контексте моего Raise метода расширения?

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

public static void Raise<T>(this object sender, ref EventHandler<T> handler, object eventLock, T e) where T : EventArgs
{
    EventHandler<T> copy;
    lock (eventLock)
    {
        copy = handler;
    }

    if (copy != null)
    {
        copy(sender, e);
    }
}

Ответы [ 7 ]

7 голосов
/ 23 января 2010

Целью блокировки является поддержание безопасности потока, когда вы переопределяете соединения событий по умолчанию. Извиняюсь, если что-то объясняет то, что вы уже смогли сделать из статьи Джона; Я просто хочу убедиться, что мне все ясно.

Если вы объявите свои события так: 

public event EventHandler Click;

Затем подписки на событие автоматически синхронизируются с lock(this). Вы не должны написать какой-либо специальный код блокировки для вызова обработчика событий. Вполне приемлемо написать: 

var clickHandler = Click;
if (clickHandler != null)
{
    clickHandler(this, e);
}

Однако , если вы решите переопределить события по умолчанию, т. Е .:

public event EventHandler Click
{
    add { click += value; }
    remove { click -= value; }
}

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

public event EventHandler Click
{
    add
    {
        lock (someLock)      // Normally generated as lock (this)
        {
            _click += value;
        }
    }
    remove
    {
        lock (someLock)
        {
            _click -= value;
        }
    }
}

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

protected virtual void OnClick(EventArgs e)
{
    EventHandler handler = _click;
    if (handler != null)
    {
        handler(this, e);
    }
}

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

Если какой-то код, внешний по отношению к классу, выходит: 

MyControl.Click += MyClickHandler;

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

Назначение переменной часть clickHandler = _click является атомарным, да, но во время этого присваивания поле _click может находиться в переходном состоянии, которое наполовину записано внешним классом. Когда вы синхронизируете доступ к полю, недостаточно только синхронизировать доступ на запись, вам также нужно синхронизировать доступ на чтение: 

protected virtual void OnClick(EventArgs e)
{
    EventHandler handler;
    lock (someLock)
    {
        handler = _click;
    }
    if (handler != null)
    {
        handler(this, e);
    }
}

UPDATE

Оказывается, что некоторые диалоги вокруг комментариев были действительно правильными, что подтверждается обновлением ОП. Это не проблема с методами расширения как таковыми, это факт, что делегаты имеют семантику типа значения и копируются при назначении. Даже если вы извлечете this из метода расширения и просто вызовете его как статический метод, вы получите то же поведение.

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

public static void RaiseEvent(ref EventHandler handler, object sync,
    object sender, EventArgs e)
{
    EventHandler handlerCopy;
    lock (sync)
    {
        handlerCopy = handler;
    }
    if (handlerCopy != null)
    {
        handlerCopy(sender, e);
    }
}

Эта версия работает, потому что мы на самом деле не передаем EventHandler, просто ссылку на него ( обратите внимание на ref в сигнатуре метода ). К сожалению, вы не можете использовать ref с this в методе расширения, поэтому он должен оставаться простым статическим методом.

(И, как уже говорилось ранее, вы должны убедиться, что вы передаете тот же объект блокировки, что и параметр sync, который вы используете в ваших публичных событиях; если вы передаете любой другой объект, тогда все обсуждение будет спорным) 1069 *

2 голосов
/ 24 мая 2017

В c # новая лучшая практика: 

  public static void Raise<T>(this EventHandler<T> handler,
  object sender, T e) where T : EventArgs
  {
     handler?.Invoke(sender, e);
  }

Вы можете увидеть эту статью.

2 голосов
/ 23 января 2010

Я понимаю, что не отвечаю на ваш вопрос, но более простой подход к устранению возможности исключения нулевой ссылки при создании события состоит в том, чтобы установить все события равными делегату {} на месте их объявления. Например: 

public event Action foo = delegate { };
1 голос
/ 25 января 2010

«Потокобезопасные» события могут стать довольно сложными. Есть несколько разных проблем, с которыми вы можете столкнуться:

NullReferenceException

Последний подписчик может отказаться от подписки между вашей нулевой проверкой и вызовом делегата, вызывая исключение NullReferenceException. Это довольно простое решение, вы можете заблокировать сайт вызова (не очень хорошая идея, поскольку вы вызываете внешний код) 

// DO NOT USE - this can cause deadlocks
void OnEvent(EventArgs e) {
    // lock on this, since that's what the compiler uses. 
    // Otherwise, use custom add/remove handlers and lock on something else.
    // You still have the possibility of deadlocks, though, because your subscriber
    // may add/remove handlers in their event handler.
    //
    // lock(this) makes sure that our check and call are atomic, and synchronized with the
    // add/remove handlers. No possibility of Event changing between our check and call.
    // 
    lock(this) { 
       if (Event != null) Event(this, e);
    }
}

Скопируйте обработчик (рекомендуется) 

void OnEvent(EventArgs e) {
    // Copy the handler to a private local variable. This prevents our copy from
    // being changed. A subscriber may be added/removed between our copy and call, though.
    var h = Event;
    if (h != null) h(this, e);
}

или иметь делегата Null, который всегда подписан. 

EventHandler Event = (s, e) => { }; // This syntax may be off...no compiler handy

Обратите внимание, что для варианта 2 (копировать обработчик) блокировка не требуется - поскольку копия является атомарной, здесь нет шансов на несогласованность.

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

void Raise(this EventHandler handler, object sender, EventArgs e) {
    if (handler != null) handler(sender, e);
}

Возможно, проблема в том, что JITter вставляет и удаляет временную переменную. Мое ограниченное понимание состоит в том, что это допустимое поведение для

устаревшие данные

Хорошо, мы решили NRE, взяв копию обработчика. Теперь у нас есть второй выпуск устаревших данных. Если подписчик откажется от подписки между нами, берущими копию и вызывающими делегата, то мы все равно будем их вызывать. Возможно, это неправильно. Вариант 1 (блокировка места вызова) решает эту проблему, но с риском тупика. Мы застряли - у нас две разные проблемы, требующие двух разных решений для одного и того же куска кода.

Поскольку тупики действительно сложно диагностировать и предотвратить, рекомендуется использовать вариант 2. Для этого необходимо, чтобы вызываемый должен обрабатывать вызовы даже после отписки. Для обработчика должно быть достаточно легко проверить, что он все еще хочет / может быть вызван, и завершить работу, если нет.

Хорошо, почему Джон Скит предлагает заблокировать OnEvent? Он препятствует тому, чтобы кэшированное чтение стало причиной устаревших данных. Призыв к блокировке преобразуется в Monitor.Enter / Exit, который создает барьер памяти, предотвращающий переупорядочение операций чтения / записи и кэшированных данных. Для наших целей они, по сути, делают делегат энергозависимым - это означает, что он не может быть кэширован в регистре ЦП и должен каждый раз считываться из основной памяти для обновления значения. Это предотвращает проблему отмены подписки подписчиком, но значение Event кэшируется потоком, который никогда не замечает.

Заключение

Итак, что насчет вашего кода: 

void Raise(this EventHandler handler, object @lock, object sender, EventArgs e) {
     EventHandler handlerCopy;
     lock (@lock) {
        handlerCopy = handler;
     }

     if (handlerCopy != null) handlerCopy(sender, e);
}

Ну, вы берете копию делегата (на самом деле дважды) и выполняете блокировку, которая создает барьер памяти. К сожалению, ваша блокировка используется при копировании вашей локальной копии - что ничего не изменит с проблемой устаревших данных, которую пытается решить Джон Скит. Вам нужно что-то вроде: 

void Raise(this EventHandler handler, object sender, EventArgs e) {
   if (handler != null) handler(sender, e);
}

void OnEvent(EventArgs e) {
   // turns out, we don't really care what we lock on since
   // we're using it for the implicit memory barrier, 
   // not synchronization     
   EventHandler h;  
   lock(new object()) { 
      h = this.SomeEvent;
   }
   h.Raise(this, e);
}

, который на самом деле не выглядит для меня намного меньше кода.

1 голос
/ 23 января 2010 
lock (@lock)
{
    handlerCopy = handler;
}

Назначение базовых типов, таких как ссылки, является атомарным, поэтому здесь нет смысла использовать блокировку.

0 голосов
/ 25 января 2010

Здесь есть несколько проблем, и я буду заниматься ими по одному.

Проблема № 1: Ваш код, вам нужно заблокировать?

Прежде всего, код, который у вас есть в вашем вопросе, нет необходимости в блокировке этого кода.

Другими словами, метод Raise можно просто переписать так: 

public static void Raise(this EventHandler handler, object sender, EventArgs e)
{
    if (handler != null)
        handler(sender, e);
}

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

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

Так что это проблема № 1, вам нужно заблокировать, если у вас есть код, как вы сделали. Ответ - нет.

Проблема № 3: Почему выходные данные из вашего последнего фрагмента кода не изменились?

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

Вы можете посмотреть на это следующим образом: 

private int x;

public void Test1()
{
    x = 10;
    Test2(x);
}

public void Test2(int y)
{
    Console.WriteLine("y = " + y);
    x = 5;
    Console.WriteLine("y = " + y);
}

Ожидаете ли вы, что y изменится на 5 в этом случае? Нет, вероятно нет, и то же самое с делегатами.

Проблема № 3: Почему Джон использовал блокировку в своем коде?

Так почему же Джон использовал блокировку в своем посте : Выбор того, что блокировать на ? Ну, его код отличается от вашего в том смысле, что он нигде не передавал копию основного делегата.

В своем коде, который выглядит так: 

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

есть вероятность, что если он не использовал блокировки, а вместо этого написал код, подобный этому: 

protected virtual OnSomeEvent(EventArgs e)
{
    if (handler != null)
        handler (this, e);
}

тогда другой поток может изменить «обработчик» между вычислением выражения, чтобы выяснить, есть ли подписчики, и до фактического вызова, другими словами: 

protected virtual OnSomeEvent(EventArgs e)
{
    if (handler != null)
                         <--- a different thread can change "handler" here
        handler (this, e);
}

Если бы он передал handler в отдельный метод, его код был бы аналогичен вашему, и поэтому не требовал блокировки.

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

Одной из причин использования блокировки даже при вызове может быть введение барьера памяти, но я сомневаюсь, что это повлияет на этот код.

Итак, это проблема №3, почему код Джона действительно нуждался в блокировке.

Проблема № 4: А как насчет изменения методов доступа к событиям по умолчанию?

Проблема 4, которая была затронута в других ответах, здесь связана с необходимостью блокировки при перезаписи средств доступа по умолчанию для добавления / удаления для события, чтобы управлять логикой по любой причине.

В основном вместо этого: 

public event EventHandler EventName;

Вы хотите написать это или какой-то другой вариант: 

private EventHandler eventName;
public event EventHandler EventName
{
    add { eventName += value; }
    remove { eventName -= value; }
}

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

Мы можем получить сценарий выполнения, который выглядит следующим образом (помните, что «a + = b» действительно означает «a = a + b»): 

Thread 1              Thread 2
read eventName
                      read eventName
add handler1
                      add handler2
write eventName
                      write eventName  <-- oops, handler1 disappeared

Чтобы это исправить, вам нужна блокировка.

0 голосов
/ 24 января 2010

Я не уверен в правильности взятия копии, чтобы избежать нулей. Событие становится нулевым, когда все подписчики говорят вашему классу не разговаривать с ними. null означает никто не хочет слышать ваше событие. Возможно, прослушивание объекта было только что удалено. В этом случае копирование обработчика просто снимает проблему. Теперь вместо вызова нулевого значения вы получаете вызов обработчику событий, который пытался отписаться от события. Вызов скопированного обработчика просто перемещает проблему от издателя к подписчику.

Я предлагаю просто попробовать / поймать; 

    if (handler != null)
    {
        try
        {
            handler(this, e);
        }
        catch(NullReferenceException)
        {
        }
    }

Я также подумал, что проверю, как Microsoft поднимет самое важное событие из всех; нажимая кнопку Они просто делают это на базе Control.OnClick; 

protected virtual void OnClick(EventArgs e)
{
    EventHandler handler = (EventHandler) base.Events[EventClick];
    if (handler != null)
    {
        handler(this, e);
    }
}

Итак, они копируют обработчик, но не блокируют его.

...