[#2275] Drop redundant consumers monitor from topic dispatch empty-check

[mattrpav]

Currently, there is a synchronized guard around consumers being empty to fire an advisory. The consumers collection is protected final and uses a CopyOnWriteArrayList.

The risk to anyone extending Topic would be if they override the consumers collection with a non-thread safe collection type, they may miss an advisory for a message arriving when there are no consumers.

[cshannon]

The reason for the lock is not just the isEmpty() but for also executing onMessageWithNoConsumers() under lock. There's lots of plugins (including people plugging in custom behavior) that has relied on executing knowing the consumers list isn't being changed.

This might be fine because as you said it's mostly just for the advisory firing, but I would want to take a closer look, I would in general cautious about being over eager to "optimize" things unless we are showing real bottlenecks and not just theoretical benchmark stuff. It's pretty easy to introduce unintended thread safety bugs with a chance like this.

If for some reason we still wanted to lock when running onMessageWithNoConsumers(), maybe we could at least do:

if (consumers.isEmpty()) {
    synchronized (consumers) {
        if (consumers.isEmpty()) {
            onMessageWithNoConsumers(context, message);
            return;
        }
    }
}

[mattrpav]

Double-check is a good pattern. There is also some inconsistency with how the call for onMessageNoConsumers is called. Further down after checking the dispatchPolicy, there is a call outside the consumers monitor.

if (!dispatchPolicy.dispatch(message, msgContext, consumers)) {
    onMessageWithNoConsumers(context, message);
}