How to declare generic multitype collection of generic handlers

Question

I always have hard time using generics with collections and wildcards.

So here is the following map. I want to keep collection of handlers for a specific type of packet class.

private ConcurrentHashMap<Class<? extends Packet>, List<PacketListener<? extends Packet>>> listeners = new ConcurrentHashMap<>();

And the PacketListener

public interface PacketListener<T extends Packet> {

    public void onOutgoingPacket(Streamer streamer, T packet);

    public void onIncomingPacket(Streamer streamer, T packet);
}

now what I would like to do is to get listeners depending on incoming packet class like this:

public <T extends Packet> void addPacketListener(Class<T> clazz, PacketListener<T> listener) {
    if (listeners.containsKey(clazz) == false) {
        listeners.putIfAbsent(clazz, new LinkedList<PacketListener<T>>());  // ERROR
    }
    List<PacketListener<? extends Packet>> list = listeners.get(clazz);
    list.add(listener);
}

public <T extends Packet> List<PacketListener<T>> getPacketListeners(Class<T> clazz) {
    List<PacketListener<T>> list = listeners.get(clazz);// ERROR
    if (list == null || list.isEmpty()) {
        return null;
    } else {
        return new ArrayList<>(list);
    }
}

And finally I would like to perform such invocation

private <T extends Packet> void notifyListeners(T packet) {
    List<PacketListener<T>> listeners = streamer.getPacketListeners(packet.getClass());
    if (listeners != null) {
        for (PacketListener<? extends Packet> packetListener : listeners) {
            packetListener.onIncomingPacket(streamer, packet);
        }
    }
}

All I am getting are just lot of errors. Is it because of wildcards in collection declaration? Is it possible to achieve such solution?

Answer 1

There is a nice image: In one of the other answers which can explain you this problem.

The thing is called PECS which stands for

Producer extends and Consumer super.

TL;DR: you can only both add and get from/to a collection with a concrete type (T). You can get any T (and its possible subtypes) with T extends Something and you can add any Something to a Collection with T super Something but you can't go both ways: thus your errors.

Answer 2

Your issue starts here:

private ConcurrentHashMap<Class<? extends Packet>, List<PacketListener<? extends Packet>>> listeners = new ConcurrentHashMap<>();

You are expecting (or perhaps just hoping) for a way to bind the two ? together so that a lookup with a key of type Class<T> will result in a value of type List<PacketListener<T>>. Sadly there is no way to tell Java that the two ? are the same but can take different (but constrained) types.

This issue is usually solved using the covariance/contravariance methods mentioned elsewhere but in your case you need to both write and read from your collection. You therefore must use an invariance.

I believe a solution to your problem is to bind the two objects into one helper class and therefore introduce the invariance there. This way you can maintain their equality while still letting them vary under restrictions.

Some of this is a little hacky IMHO (i.e. there are some casts) but at least you can achieve your aim and you are still type safe. The casts are provably valid.

public interface PacketListener<T extends Packet> {

    public void onOutgoingPacket(Streamer streamer, T packet);

    public void onIncomingPacket(Streamer streamer, T packet);
}

/**
 * Binds the T's of Class<T> and PacketListener<T> so that we CAN assume they are the same type.
 *
 * @param <T> The type of Packet we listen to.
 */
private static class Listeners<T extends Packet> {

    final Class<T> packetClass;
    final List<PacketListener<T>> listenerList = new LinkedList<>();

    public Listeners(Class<T> packetClass) {
        this.packetClass = packetClass;
    }

    public List<PacketListener<T>> getListenerList() {
        return listenerList;
    }

    private void addListener(PacketListener<T> listener) {
        listenerList.add(listener);
    }

}
/**
 * Now we have bound the T of Class<T> and List<PacketListener<T>> by using the Listeners class we do not need to key on the Class<T>, we just need to key on Class<?>.
 */
private final ConcurrentMap<Class<?>, Listeners<?>> allListeners = new ConcurrentHashMap<>();

public <T extends Packet> List<PacketListener<T>> getPacketListeners(Class<T> clazz) {
    // Now we can confidently cast it.
    Listeners<T> listeners = (Listeners<T>) allListeners.get(clazz);
    if (listeners != null) {
        // Return a copy of the list so they cannot change it.
        return new ArrayList<>(listeners.getListenerList());
    } else {
        return Collections.EMPTY_LIST;
    }
}

public <T extends Packet> void addPacketListener(Class<T> clazz, PacketListener<T> listener) {
    // Now we can confidently cast it.
    Listeners<T> listeners = (Listeners<T>) allListeners.get(clazz);
    if (listeners == null) {
        // Make one.
        Listeners<T> newListeners = new Listeners<>();
        if ((listeners = (Listeners<T>) allListeners.putIfAbsent(clazz, newListeners)) == null) {
            // It was added - use that one.
            listeners = newListeners;
        }
    }
    // Add the listener.
    listeners.addListener(listener);
}

Note that although it is generally assumed that if you need to cast something while using generics you are doing something wrong - in this case we can be safe because of the run-time assurance that all Listeners<T> objects in the map are keyed by their Class<T> and therefore the enclosed list is indeed a List<PacketListener<T>.

Answer 3

The following is some kind of similiar to the answer of @OldCurmudgeon.

The keypoint is also the listeners field. But I declare it as this:

private final Map<Class<?>, DelegatingPacketListener> listeners

The point here is that we get rid of the list as the map value type. DelegatingPacketListener is declared as follows:

public class DelegatingPacketListener implements PacketListener<Packet> {

    private final List<PacketListener<Packet>> packetListeners;

    public DelegatingPacketListener(List<? extends PacketListener<Packet>> packetListeners) {
        super();
        this.packetListeners = new ArrayList<PacketListener<Packet>>(packetListeners);
    }

    @Override
    public void onOutgoingPacket(Streamer streamer, Packet packet) {
        for(PacketListener<Packet> packetListener : packetListeners) {
            packetListener.onOutgoingPacket(streamer, packet);
        }
    }

    @Override
    public void onIncomingPacket(Streamer streamer, Packet packet) {
        for(PacketListener<Packet> packetListener : packetListeners) {
            packetListener.onIncomingPacket(streamer, packet);
        }
    }

    public List<PacketListener<Packet>> getPacketListeners() {
        return Collections.unmodifiableList(packetListeners);
    }
}

Now that DelegatingPacketListener only supports listeners of type Packet we need one more specific implementation of PacketListener:

public class WrappingPacketListener<T extends Packet> implements PacketListener<Packet> {

    private final Class<T> packetClass;
    private final PacketListener<T> wrapped;

    public WrappingPacketListener(Class<T> packetClass, PacketListener<T> delegate) {
        super();
        this.packetClass = packetClass;
        this.wrapped = delegate;
    }

    @Override
    public void onOutgoingPacket(Streamer streamer, Packet packet) {
        if(packetClass.isInstance(packet)) {
            T genericPacket = packetClass.cast(packet);
                wrapped.onOutgoingPacket(streamer, genericPacket);
        }
    }

    @Override
    public void onIncomingPacket(Streamer streamer, Packet packet) {
        if(packetClass.isInstance(packet)) {
            T genericPacket = packetClass.cast(packet);
                wrapped.onIncomingPacket(streamer, genericPacket);
        }
    }
}

Please note that the type parameter T is not used in the implements clause. It is only for the implementation used. We will wrap every PacketListener passed to the API in a WrappingPacketListener. So the implementation is like this:

public List<PacketListener<Packet>> getPacketListeners(Class<?> clazz) {
    return Collections.<PacketListener<Packet>>singletonList(listeners.get(clazz));
}

public <T extends Packet> void addPacketListener(Class<T> clazz, PacketListener<T> listener) {
    if (listeners.containsKey(clazz) == false) {
        listeners.put(clazz, new DelegatingPacketListener(Collections.singletonList(new WrappingPacketListener<T>(clazz, listener))));
        return;
    }
    DelegatingPacketListener existing = listeners.get(clazz);
    List<PacketListener<Packet>> newListeners = new ArrayList<PacketListener<Packet>>(existing.getPacketListeners());
    newListeners.add(new WrappingPacketListener<T>(clazz, listener));
    listeners.put(clazz, new DelegatingPacketListener(newListeners));        
}

private <T extends Packet> void notifyListeners(T packet) {
    List<PacketListener<Packet>> listeners = streamer.getPacketListeners(packet.getClass());
    if (listeners != null) {
        for (PacketListener<Packet> packetListener : listeners) {
            packetListener.onIncomingPacket(streamer, packet);
        }
    }
}

The API has slightly changed for getPacketListeners which doesnt use a generic type anymore.

In comparison with OldCurmudgeon's solution, this one sticks with the already existing PacketListener interface and doesn't require an unchecked cast to be applied.

Note that the implementation is not thread safe, because of the implemention of addPacketListener needs synchronization on the map key (as the original code in question does need too). However encapsulating the list of packet listeners in immutable DelegatingPacketListener is probably better suited for concurrency purposes.