5
\$\begingroup\$

I'm working on an event manager, and I am wanting it to be a tool developers use. It is lightweight and it uses annotations to register events. I've tried to set up synchronization, and I'd like to ask if I'm doing it right so far. It's on GitHub.

The main class that I would worry about for synchronization would be the EventManager class:

// JavaDocs at GitHub Repo (if needed)
public class EventManager {

    private static volatile Logger logger = Logger.getLogger("EGEventManager");

    private static volatile List<Class<? extends Event>> eventClasses = new ArrayList<>();
    private static volatile List<EventListener> registeredListeners = new ArrayList<>();

    ...
    public static synchronized boolean registerEventClass(Class<? extends Event> event) {
        if (!eventClasses.contains(event)) {
            eventClasses.add(event);
            return true;
        }
        return false;
    }

    ...
    public static synchronized boolean unregisterEventClass(Class<? extends Event> event) {
        if (eventClasses.contains(event)) {
            eventClasses.remove(event);
            return true;
        }
        return false;
    }

    ...
    public static synchronized boolean isEventClassRegistered(Class<? extends Event> event) {
        return eventClasses.contains(event);
    }

    ...
    public static synchronized List<RegisteredEvent> registerEventListener(EventListener listener) {
        List<RegisteredEvent> newlyRegistered = null;
        if (!registeredListeners.contains(listener)) {
            newlyRegistered = registerEventHandlers(listener);
        }
        return newlyRegistered;
    }

    @SuppressWarnings("unchecked")
    private static synchronized List<RegisteredEvent> registerEventHandlers(EventListener listener) {
        List<RegisteredEvent> newlyRegistered = new ArrayList<>();

        try {
            Class<? extends EventListener> eventListenerClass = listener.getClass();
            Method[] classMethods = eventListenerClass.getDeclaredMethods();
            for (int i = 0; i < classMethods.length; i++) {
                Method method = classMethods[i];
                if (method.getParameterCount() != 1) continue;
                EventHandler[] methodAnnotations = method.getDeclaredAnnotationsByType(EventHandler.class);
                if (methodAnnotations.length == 0) continue;
                EventHandler eventHandlerAnnotation = methodAnnotations[0];
                EventPriority priority = eventHandlerAnnotation.value();
                Class<? extends Event> eventClass = (Class<? extends Event>) method.getParameterTypes()[0];
                PrioritizedEvents.addRegisteredEvent(new RegisteredEvent(listener, method, eventClass, priority));
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
        return newlyRegistered;
    }

    ...
    public static synchronized boolean unregisterEventListener(EventListener listener) {
        if (registeredListeners.contains(listener)) {
            registeredListeners.remove(listener);
            return true;
        }
        return false;
    }

    ...
    public static synchronized void call(EventExecutor eventExecutor, Class<? extends Event> eventClass, Object... eventArgs) {
        try {
            if (!eventClasses.contains(eventClass)) {
                logger.warning("EventManager.call(Class<? extends Event>) cancelled: event is not contained in the registered Event classes!");
                return;
            }

            List<RegisteredEvent> lowPriority = PrioritizedEvents.getRegisteredEvents(EventPriority.Low);
            List<RegisteredEvent> normalPriority = PrioritizedEvents.getRegisteredEvents(EventPriority.Normal);
            List<RegisteredEvent> highPriority = PrioritizedEvents.getRegisteredEvents(EventPriority.High);

            Event eventInstance = null;

            Class<?>[] constructorParameters = EventUtilities.getArrayOfClasses(eventArgs);
            Constructor<?> constructor = eventClass.getDeclaredConstructor(constructorParameters);

            eventInstance = (Event) constructor.newInstance(eventArgs);

            for (int i = 0; i < lowPriority.size(); i++) {
                RegisteredEvent registeredEvent = lowPriority.get(i);
                EventListener listener = registeredEvent.getListener();
                Method method = registeredEvent.getMethod();

                if (registeredEvent.getEventClass() != eventClass) continue;
                method.invoke(listener, eventInstance);
            }
            for (int i = 0; i < normalPriority.size(); i++) {
                RegisteredEvent registeredEvent = normalPriority.get(i);
                EventListener listener = registeredEvent.getListener();
                Method method = registeredEvent.getMethod();

                if (registeredEvent.getEventClass() != eventClass) continue;
                method.invoke(listener, eventInstance);
            }
            for (int i = 0; i < highPriority.size(); i++) {
                RegisteredEvent registeredEvent = highPriority.get(i);
                EventListener listener = registeredEvent.getListener();
                Method method = registeredEvent.getMethod();

                if (registeredEvent.getEventClass() != eventClass) continue;
                method.invoke(listener, eventInstance);
            }

            if (!eventInstance.isCancelled()) eventExecutor.execute();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    ...
    static class PrioritizedEvents {

        private static volatile Map<EventPriority, List<RegisteredEvent>> prioritized = new HashMap<>();

        static {
            EventPriority[] values = EventPriority.values();
            for (int i = 0; i < values.length; i++) {
                EventPriority priority = values[i];
                prioritized.put(priority, new ArrayList<>());
            }
        }

        ...
        public static synchronized List<RegisteredEvent> getRegisteredEvents(EventPriority priority) {
            return prioritized.get(priority);
        }

        ...
        public static synchronized boolean addRegisteredEvent(RegisteredEvent registeredEvent) {
            getRegisteredEvents(registeredEvent.getPriority()).add(registeredEvent);
            return true;
        }
    }

Things in this EventManager class like the java.util.Logger probably don't need to be volatile, but please correct me on this.

For some things like the RegisteredEvent class, I expect that it would need to be synchronized. But in some of the classes, I'm thinking not everything needs to be tagged with the volatile keyword.

// Full code available at GitHub (if needed)
public class RegisteredEvent {

    private EventListener listener;
    private Method method;
    private Class<? extends Event> eventClass;
    private EventPriority priority;

    ...
    public RegisteredEvent(EventListener listener, Method method, Class<? extends Event> eventClass, EventPriority priority) {
        this.listener = listener;
        this.method = method;
        this.eventClass = eventClass;
        this.priority = priority;
    }

    ...
    public EventListener getListener() {
        return listener;
    }

    ...
    public Method getMethod() {
        return method;
    }

    ...
    public Class<?> getEventClass() {
        return eventClass;
    }

    ...
    public EventPriority getPriority() {
        return priority;
    }
}
\$\endgroup\$
2
\$\begingroup\$
  1. There is no need to use the volatile keyword in the EventManager class. All methods are synchronized, so it is thread-safe(it is guaranteed that only one thread can execute only one method at a time and all changes made by this thread are visible to other threads that execute any method of this class in the future).

  2. Some things like the RegisteredEvent class, I expect that it would need to be synchronized, but in some of the classes, I'm thinking not everything needs to be tagged with the volatile keyword.

    Thread-safety is about controlling an access to a shared state. It's up to you to specify which parts of you system can be accessed from multiple threads(and how) and make decisions based on that.

  3. Do not arbitrary tag variables with the volatile keyword. Do not arbitrary add the synchronized keyword to some methods of some classes. You are very unlikely to get it right this way. The best way to go is to get a solid understanding of the Java memory model.

\$\endgroup\$
4
\$\begingroup\$

In Java, there are essentially 4 ways of handling thread safety:

  1. use only one thread, ever.
  2. use synchronization, correctly.
  3. use constructs in the java.util.concurent.* packages, correctly
  4. use volatile

It is almost always a problem if you see multiple of these systems in the same class. Thread safety is complicated. Each one of those strategies has complications, and combining them in one place compounds the complications, and never simplifies them.

Volatile

Further, in Java, the volatile concept is hard to describe, and use cases for it are limited, and it almost never solves the problems you think it does. Additionally, volatile semantics changed in Java 1.4 and what it did before that is different to what it does now.

Worse, in your code, the usage of volatile solves nothing, and just makes things slower.

I would even go so far as to say that any time you think you want to use volatile, you are wrong. Think of volatile as being like the "GOTO" of concurrency. Sure, there may be cases where it is useful, but normally there is a better way to do it:

enter image description here

In your case, volatile does nothing useful, and lots of things that are bad. Remove it.

Synchronization

Synchronized methods are another red flag in multi-threaded Java. synchronization consists of two parts, a "monitor", and a "code block". The "Monitor" is something Java can find in memory, and is used to control access to the code block. In Java terms, the Monitor is always an object. When Java is running and it encounters a synchronized code block, it (conceptually does, the fine details are more complicated):

  1. looks at the monitor location for the code block.
  2. checks to see if any other thread has the monitor locked.
  3. waits until the monitor is unlocked
  4. gets a 'clean' copy of all the variables used inside the code block from a "good source".
  5. runs the code inside the block
  6. updates the "good source" with the new values for any variables changed inside the code block.
  7. unlocks the monitor

Note, there are a few key items in there... (and also note, that you cannot describe all of the Java memory model in 7 bullet points).

The first thing to note, though, is that there is a monitor, and that anyone who uses that monitor as a lock will have to wait for anyone else. In your code, you have:

public static synchronized boolean registerEventClass(
    ...

What is the monitor used for that? Well, it is the class itself: RegisteredEvent.class. Now, this may not seem to be a bad thing, but, what if someone else does:

while (true) {
    synchronized(RegisteredEvent.class) {
        Thread.sleep(10000);
    }
}

Now, your code will only get an opportunity to run every 10 seconds ;-) Although this seems contrived, I have encountered a number of situations where monitors are used in places where you don't expect them, leading to hard-to-debug concurrency problems.

What's the solution? Well, that's easy, create a special lock instance:

private static final Object syncLock = new Object();

Then, because that is private, noone else can ever lock on it, only you. Who knew that bare Objects would ever be useful? Now you can use it like:

public static boolean registerEventClass(
    synchronized(syncLock) {
        ...

Bottom line, it is almost always a potential bug to have a synchronized method on your code, unless you fully intend for other programmers to be able to impact the concurrency model of your code. This applies for non-static methods too. In instance methods, create an instance lock:

private final Object syncLock = new Object();

....

public int getValue() {
    synchronized(syncLock) {
        ....
    }
}

That way, you are always in control.

\$\endgroup\$
  • 1
    \$\begingroup\$ I will take into consideration for removing volatile and using a synchronized lock, thank you. \$\endgroup\$ – CoderMusgrove Mar 3 '15 at 19:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.