[#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);
}

[mattrpav]

@cshannon what are your thoughts on aligning the call sites for 'onMessageWithNoConsumers'?

I'm leaning towards refactoring all call sites to not have any locking. I don't see how it matters -- if a message comes in and there are no consumers, then run the onMessageWithNoConsumers path. If a consumer is added in the meantime, what does that change?

When onMessageWithNoConsumers is run within the lock of consumers, its not as if any back-off or retry logic can run to try to wait for a consumer to show up b/c the consumers collection is blocking any consumers from being added -- if anything, this could lead to more messages being missed by delaying a consumer from being activated and catching the 'next' message as fast as possible.

Currently, that is a protected method on BaseDestination, so not really a 'pluggable' entry point there. The BaseDestination impl does call up to the broker-level onMessageWithNoConsumers, which is pluggable, so your comment about being called within the state of there being no consumers is correct; however, the inconsistency of the call sites shows that is an incomplete contract of sorts.

Side note -- From a design perspective, this should probably be relocated to the DeadLetterStrategy as it is in the same category of 'I got a message and currently no plans to do anything with it' type deal to provide a way to plugin at the destination layer.