SimpleApplicationEventMulticaster is not thread safe!

[mlavwilson2]

I want to register a new eventHandler in an event, I get a java.util.ConcurrentModificationException upon doing this. Is there a reason that this is implemented in this way?

I had to write my own ApplicationEventMulticaster to get this working...

package net.mlw.newsfutures.gui;

import java.util.HashSet;
import java.util.Set;

import org.springframework.context.ApplicationEvent;
import org.springframework.context.ApplicationListener;
import org.springframework.context.event.ApplicationEventMulticaster;

public class SimpleApplicationEventMulticaster implements ApplicationEventMulticaster
{
   /** Set of listeners */
   //private final Set applicationListeners = new HashSet();
   private final List applicationListeners = new ArrayList();

   public synchronized void addApplicationListener(ApplicationListener listener)
   {
      this.applicationListeners.add(listener);
   }

   public synchronized void removeApplicationListener(ApplicationListener listener)
   {
      this.applicationListeners.remove(listener);
   }

   public synchronized void removeAllListeners()
   {
      this.applicationListeners.clear();
   }

   public synchronized ApplicationListener[] getApplicationListeners()
   {
      return (ApplicationListener[]) applicationListeners.toArray(new ApplicationListener[applicationListeners.size()]);
   }

   public void multicastEvent(ApplicationEvent event)
   {
      ApplicationListener[] listeners = getApplicationListeners();

      for (int i = 0, length = listeners.length; i < length; i++)
      {
         listeners[i].onApplicationEvent(event);
      }
   }
}

[oliverhutchison]

I've found that as a general rule it's a good idea to look at the JavaDocs for abstract superclasses particularly in the Spring code base. A quick look at the JavaDocs for AbstractApplicationEventMulticaster reveals:

Note that this class doesn't try to do anything clever to ensure thread
safety if listeners are added or removed at runtime. A technique such as
Copy-on-Write (Lea:137) could be used to ensure this, but the assumption in
the basic version of the class is that listeners will be added at application
configuration time and not added or removed as the application runs.

So I guess the code is not so poor after all.

PS. have you actualy tried to remove a listener while delivering events with your example code? I'd argue that the ArrayIndexOutOfBoundsException which you may/or may not get is worse than a ConcurrentModificationException. And when you add a listener while delivering events you may end up delivering the event to the some listeners twice and not delivering to others at all.

[mlavwilson2]

PS. have you actualy tried to remove a listener while delivering events with your example code? I'd argue that the ArrayIndexOutOfBoundsException which you may/or may not get is worse than a ConcurrentModificationException. And when you add a listener while delivering events you may end up delivering the event to the some listeners twice and not delivering to others at all.

My impl is over synchronized, I don't see how a problem could occure.

[oliverhutchison]

My impl is over synchronized, I don't see how a problem could occure.

You did see the problem - I see that you've edited the bug out of your original post.

[mlavwilson2]

I did not think of the bug caused by removing a listener. Adding would work, since I do not wat the added one to be fired duting the current event.

public class SimpleApplicationEventMulticaster implements ApplicationEventMulticaster
{
   /** Set of listeners */
   private final Set applicationListeners = new HashSet();
   private ApplicationListener[] listeners = null;

   public synchronized void addApplicationListener(ApplicationListener listener)
   {
      this.applicationListeners.add(listener);
      this.listeners = (ApplicationListener[]) applicationListeners.toArray(new ApplicationListener[applicationListeners.size()]);
   }

   public synchronized void removeApplicationListener(ApplicationListener listener)
   {
      this.applicationListeners.remove(listener);
      this.listeners = (ApplicationListener[]) applicationListeners.toArray(new ApplicationListener[applicationListeners.size()]);
   }

   public synchronized void removeAllListeners()
   {
      this.applicationListeners.clear();
      this.listeners = null;
   }

   public synchronized void multicastEvent(ApplicationEvent event)
   {
      if (listeners != null)
      {
         for (int i = 0, length = listeners.length; i < length; i++)
         {
            listeners[i].onApplicationEvent(event);
         }
      }
   }
}

[oliverhutchison]

Adding would work, since I do not wat the added one to be fired duting the current event.

Your code assumes that the order of items returned by HashSet#toArray is the same as the order the items were inserted into the set. This is not correct - the order of the items returned by HashSet#toArray is based on which buckets the items ended up being hashed into when they were inserted or last time they were re-hashed which, with good hash functions, basically boils down to being random.

So what's going to happen with an add is that the listeners array is going to be semi-shuffled rather then just having a new item added to the end.

It's becuase of these kind of issues that you should at all costs avoid writing your own sycronized code. Though if you do need to write your own code make sure you unit test it hard.

So rather then writing your own thread safe version of SimpleApplicationEventMulticaster you're probably better off just doing this:

<bean id="applicationEventMulticaster" class="org.springframework.context.event.SimpleApplicationEventMulticaster">
    <property name="collectionClass" value="java.util.concurrent.CopyOnWriteArraySet" />
</bean>

Ollie