Simple Publisher Subscriber in Java

Question

I am new to Java and had some difficulty creating this simple Publisher Subscriber class. It finally seems to be working and I wanted to check if I have been following best practices and see if there are any improvements I can make.

The following Java class accepts any topics as a string (e.g. "rand_int") and forwards any messages to that topic to any subscribers.

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;

public class PubSub {
  private Map<String, ArrayList<Consumer<?>>> topicsSubscribers =
      new HashMap<String, ArrayList<Consumer<?>>>();
  private static PubSub pubSubSingleton = null;

  public static PubSub getInstance() {
    if (pubSubSingleton == null)
      pubSubSingleton = new PubSub();

    return pubSubSingleton;
  }

  public <T> void publish(String topic, T message) {
    ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topic);
    if (subscribers == null)
      return;

    for (Consumer subscriberConsumer : subscribers) {
      subscriberConsumer.accept(message);
    }
  }

  public synchronized <T> void subscribe(String topicName, Consumer<T> subscriberCallback) {
    ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topicName);
    if (subscribers == null) {
      subscribers = new ArrayList<Consumer<?>>();
      subscribers.add(subscriberCallback);
      this.topicsSubscribers.put(topicName, subscribers);
    } else {
      subscribers.add(subscriberCallback);
    }
  }
}

Example usage:

public class SomeClass {
  public SomeClass() {
    PubSub.getInstance().subscribe("rand_int", this::randIntCallback);
    PubSub.getInstance().publish("rand_int", 46);
  }

  public randIntCallback(int number) {
    System.out.println(number);
  }
}

The above code should output "46".

As I said, I am very new to Java and would like some feedback on my code to help improve. So any help on this is very appreciated.

Answer 1

Initializing object variables

  private Map<String, ArrayList<Consumer<?>>> topicsSubscribers =
      new HashMap<String, ArrayList<Consumer<?>>>();

The standard is to set the types as the interfaces, like Map and List.

    private final Map<String, List<Consumer<?>>> topicSubscribers = new HashMap<>();

Also, you don't need to specify the contents of <> in the implementation instantiation. The compiler will work that out for you.

Initializing class variables

  private static PubSub pubSubSingleton = null;

  public static PubSub getInstance() {
    if (pubSubSingleton == null)
      pubSubSingleton = new PubSub();

    return pubSubSingleton;
  }

You don't have to do it this way. You can simply say

    private static final PubSub pubSubSingleton = new PubSub();

    public static PubSub getInstance() {
        return pubSubSingleton;
    }

    private PubSub() {
    }

This saves the null check each time while still only initializing once.

However, Singleton is generally considered an anti-pattern now. You may want to consider alternatives. One possibility would be a separate instance for each message type.

Your original code does not make the constructor private.

The Java standard is four column indents rather than the two you are using.

Formatting

    if (subscribers == null)
      return;

If you are going to use the statement form, you should probably keep it one line:

    if (subscribers == null) return;

If it's too complicated to fit into one line, you should probably use the block form instead.

I actually use the block form consistently regardless.

    if (subscribers == null) {
        return;
    }

Factor out the common

    if (subscribers == null) {
      subscribers = new ArrayList<Consumer<?>>();
      subscribers.add(subscriberCallback);
      this.topicsSubscribers.put(topicName, subscribers);
    } else {
      subscribers.add(subscriberCallback);
    }

This could be

    if (subscribers == null) {
        subscribers = new ArrayList<Consumer<?>>();
        topicsSubscribers.put(topicName, subscribers);
    }

    subscribers.add(subscriberCallback);

You don't have to do the add before the put. It's the same object either way. Otherwise, you'd have to do the put after the add in the non-null case.

Answer 2

We consider singletons bad (i.e. your getInstance bit). There are numerous resources on why a short search away so I'm not going to list them here.

That said your getInstance is not safe in a threaded context. You need to use Double-checked locking. Note that I do think that initialising the object in the static constructor or inline with the declaration is safe as well.

Your subscribe method is synchronized on the class object but the publish method is not meaning that you have a data race where the subscribers for a topic may change in the middle of iterating over it, typically this will throw a ConcurrentModificationException when it happens. Either you do a thread safe design and add synchronized to publish as well or do a thread-un-safe design and remove the synchronized keyword everywhere.

Your publish method swallows any errors. This is not good. Depending on your philosophy on error reporting you should either return a boolean signifying the success or failure of publishing the message to at least one receiver or throw an exception. In this case I would return an error, or maybe even the number of subscribers that the message was delivered too, yeah I think that makes more sense and is cheap to compute and rich in information.

And finally, your class will forever hold a reference to the subscriber object and is likely to cause a memory leak as you cannot remove them, especially since this is a singleton which has the same lifetime as your application. A common pattern is to either allow removal through unsubscribe or use WeakReference and let the life time be tied to an external object with cleanup of any weak references whose get returns null (object removed/gcd)

Answer 3

One small addition to what the others said:

ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topicName);
if (subscribers == null) {
  subscribers = new ArrayList<Consumer<?>>();
  subscribers.add(subscriberCallback);
  ...

is an outdated pattern. Since java 8 (i.e. 4.5 years now) we have computeIfAbsent in the Map. Therefore, use:

ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.computeIfAbsent(sub -> new ArrayList<>());

instead.