EventAggregator, is it thread-safe?

Question

Is this thread-safe?

The EventAggregator in Prism is a very simple class with only one method. I was surprised when I noticed that there was no lock around the null check and creation of a new type to add to the private _events collection. If two threads called GetEvent simultaneously for the same type (before it exists in _events) it looks like this would result in two entries in the collection.

    /// <summary>
    /// Gets the single instance of the event managed by this EventAggregator. Multiple calls to this method with the same <typeparamref name="TEventType"/> returns the same event instance.
    /// </summary>
    /// <typeparam name="TEventType">The type of event to get. This must inherit from <see cref="EventBase"/>.</typeparam>
    /// <returns>A singleton instance of an event object of type <typeparamref name="TEventType"/>.</returns>
    public TEventType GetEvent<TEventType>() where TEventType : EventBase
    {
        TEventType eventInstance = _events.FirstOrDefault(evt => evt.GetType() == typeof(TEventType)) as TEventType;
        if (eventInstance == null)
        {
            eventInstance = Activator.CreateInstance<TEventType>();
            _events.Add(eventInstance);
        }
        return eventInstance;
    }

Moussaka

Answer 1

No, not thread safe.

1. The instance member access of the List class itself are NOT thread safe, as per MSDN under Thread safety
2. The method itself is not thread safe
   1. 2 threads could enter the method at the same time
   2. Both try to get the FirstOrDefault
   3. Both get nothing
   4. Both add a new TEventType

I would

1. switch to one of the System.CollectionConcurrentX collections in .NET 4
   http://msdn.microsoft.com/en-us/library/system.collections.concurrent.aspx
   or
2. do your own locking

Sacculate

+1 @Peter Agreed. I came here to sanity check after having some issues and then looking at the code for EventAggregator in reflector. I cannot believe that they did not make it thread-safe given the intended architectural use. I was surprised anyway. – Scarificator

@chibacity me too, surprised I was ;-) – Sacculate

Answer 2

It depends what "_events" is...

There are some awesome new thread-safe classes in .NET 4 ...
Check out http://msdn.microsoft.com/en-us/library/system.collections.concurrent.aspx

Sacculate

sorry, I should have included it. It's just a list : private readonly List<EventBase> _events = new List<EventBase>(); – Moussaka

Answer 3

Well, based on that code paste, I'd say no, it's not 100% thread safe.

Of course, you have the source, so you can just add the lock yourself. :)

Actually, as a general rule of thumb, I include the entire CAL project in my solution, at least at the start. It helps a lot in debugging those odd region registration/creation exceptions...

Vida