Getting a set of subscribers from a subscriberMap

Question

In my Java event bus project, I have a private generic method that retrieves a Set<Subscriber<E>> from a private Map<Class<? extends Event>, Set<Subscriber<? extends Event>>> (the subscriberMap).

/**
 * A map of Event types and Subscriber Sets.
 * 
 * That is, the Set of Subscribers set to an Event type
 * is assumed to be used for that Event type.
 */
private final Map<Class<? extends Event>, Set<Subscriber<? extends Event>>> subscriberMap = new ConcurrentHashMap<>();

private <E extends Event> void checkSubscriberMap(Class<E> type)
{
    if (!subscriberMap.containsKey(type))
    {
        subscriberMap.put(type, new HashSet<>());
    }
}

/**
 * Gets a Set of Subscribers by an Event.
 *
 * @param event the event
 * @param <E> the type of event
 * @return a Set of Subscribers
 */
@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(E event)
{
    return (Set<Subscriber<E>>) (Object) getSubscribers(event.getClass());
}

/**
 * Gets a Set of Subscribers by an Event type.
 *
 * @param type the event class
 * @param <E> the type of event
 * @return a Set of Subscribers
 */
@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(Class<E> type)
{
    checkSubscriberMap(type); // checks if subscriberMap.get(type) is null

    return (Set<Subscriber<E>>) (Object) subscriberMap.get(type);
}

Assuming nobody uses reflection to modify subscriberMap, the public methods that modify subscriberMap only work if the Subscriber<? extends Event> is the same type as the Class<? extends Event>. Therefore, I can be sure that getSubscribers() will always return a Set of the same type that you give it.

/**
 * Registers a single event handler under an explicit Event type.
 * Returns true if the handler was added.
 *
 * @param subscriber the Subscriber
 * @param type the type of Event
 */
public <E extends Event> boolean register(Subscriber<E> subscriber, Class<E> type)
{
    if(type == Event.class)
    {
        throw new IllegalArgumentException("The provided type is not a subclass of Event.");
    }

    return getSubscribers(type).add(subscriber);
}

However, it's bothering me how getSubscribers() looks. Why does it look I'm doing a hacky workaround? Is it necessary that I have to cast subscriberMap.get(type) twice? Is this the right way to go about what I'm trying to do?

Answer 1

private <E extends Event> void checkSubscriberMap(Class<E> type)

This method checks nothing. It ensures that there's a non-null value. Or it fixes or prepares something.

Note that you're using both contains and get every time. I guess, a method like

private <E extends Event> Set<E> safeGet(Class<E> type) {
   Set<E> result =
           (Set<Subscriber<E>>) (Object) subscriberMap.get(type);
   if (result == null) {
        result = new HashSet<>();
        subscriberMap.put(type, result);
   }
   return result;
}

would be better.

Consider using Guava's Multimap as it's been solved there.

---

@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(E event)
{
    return (Set<Subscriber<E>>) (Object) getSubscribers(event.getClass());
}

I'm afraid, that's right. The ugly double cast is needed as there's no way to tell the compiler that it works. Maybe the intermediate cast could be (non-generic) (Set).

---

Assuming nobody uses reflection

If somebody does, that's not your fault.

What you did is similar in idea to the MutableClassToInstanceMap, except that you need Set<Subscriber<E>> instead of E itself and therefore you can't use type.cast(object) and have to resolve to an unchecked cast.

Answer 2

Use your tools properly ;-) You are missing a bunch of functionality that is available on your class that would help you a lot.

First up, you have the following:

private final Map<Class<? extends Event>, Set<Subscriber<? extends Event>>> subscriberMap = new ConcurrentHashMap<>();

Now, that's a long line, and as a result, it took me a moment to discover that this is a ConcurrentHashMap ... this makes a big difference. So, let's use that to our advantage:

private final ConcurrentMap<Class<? extends Event>,
                            Set<Subscriber<? extends Event>>> subscriberMap
     = new ConcurrentHashMap<>();

I have exaggerated the indentation to make it all visible.....

Now, ConcurrentMap gives us some extra methods over just-plain-Map... You should use them.

First up, this "check" method. @maaartinus correctly identifies this as being a problem with the name. The standard name used in many libraries (including the Java Native libraries, is ensure......

private <E extends Event> void checkSubscriberMap(Class<E> type)
{
    if (!subscriberMap.containsKey(type))
    {
        subscriberMap.put(type, new HashSet<>());
    }
}

I would write that as:

private <E extends Event> void ensureSubscriberMap(Class<E> type) {
    subscriberMap.computeIfAbsent(type, k -> new HashSet<>());
}

Note that the supply-function is only called if the key is absent. Also, note that this is now an atomic operation, there is no race condition at all.

Now, looking through your additional code, I am perturbed by the lack of defensive copies..... The Map is concurrent, so I suspect that you have concurrency concerns... what if some other thread modifies the Subscriber sets part-way through operation? You may have a real problem. Defensive copies of data structures are normally recommended for this, but your base structure is just a HashSet, it's not thread safe. So, I would recommend a lock on that of some sort, with a thread-safe copy... also, since the 'ensure' method is now a 1-liner, I have included it directly in this method:

@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(Class<E> type) {

    // take a reference to the non-specific-generic Set 
    Set<Subscriber<? extends Event>> subs = 
             subscriberMap.computeIfAbsent(type, k -> new HashSet<>());

    // lock that set so only we can traverse it (thread safe)
    synchronized (subs) {

        // map each member to a defensive copy using the specific generic mapping.
        return subs.stream()
                   .map(s -> (Subscriber<E>)s)
                   .collect(Collectors.toSet());

    }
}