Synchronization in an event manager

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> eventClasses = new ArrayList<>();
private static volatile List registeredListeners = new ArrayList<>();

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

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

...
public static synchronized boolean isEventClassRegistered(Class event) {
return eventClasses.contains(event);
}

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

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

try {
Class eventListenerClass = listener.getClass();
Method[] classMethods = eventListenerClass.getDeclaredMethods();
for (int i = 0; i eventClass = (Class) method.getParameterTypes()[0];
PrioritizedEvents.addRegisteredEvent(new Regi

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

• use only one thread, ever.

• use synchronization, correctly.

• use constructs in the java.util.concurent.* packages, correctly

• 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:

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):

• looks at the monitor location for the code block.

• checks to see if any other thread has the monitor locked.

• waits until the monitor is unlocked

• gets a 'clean' copy of all the variables used inside the code block from a "good source".

• runs the code inside the block

• updates the "good source" with the new values for any variables changed inside the code block.

• 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.

StackExchange Code Review Q#83111, answer score: 4