WIP: [AMQ-9554] Browsing a DLQ in transacted mode should allow redelivered messages

[mattrpav]

Reproduce project: https://github.com/hyteio/hyte-activemq-redelivery-inconsistent

Summary:

Message with 7 JMSXRedeliveryCount (on the broker)

1. Browse using AUTO_ACKNOWELEDGE .. no problem
2. Browse using SESSION_TRANSACTED .. message gets poisoned ack b/c redelivery policy is being applied
3. The poison ack creates a duplicate message on the broker b/c the broker sees this is a browser, and not a consumer. So message is copied to .dlq.dlq and the original message remains in the .dlq

Proposed default behavior:

Queue Browser should be consistent regardless of ack mode when browsing messages and not send a poison ack based on redelivery policy.

Planned fix path forward:

1. Apply a new redeliveryPolicy option: queueBrowserIgnored = false (default behavior today)
2. In 7.x set queueBrowserIgnored = true (more consistent behavior for out-of-the-box JMS browse tools)

[cshannon]

This doesn't make sense to me and seems to hide the root issue. That method is simply checking if redelivery was already exceeded and handling it if it is. It also checks if the message has been expired. There's no reason to not discard the message on browse if redelivery was exceeded. Here is where expiration is also done in the same section where we discard on delivery exceeded:

activemq/activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageConsumer.java

Line 510 in e45ee4a

} else if (consumeExpiredMessage(md)) {

and

activemq/activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageConsumer.java

Line 1432 in e45ee4a

boolean expired = isConsumerExpiryCheckEnabled() && message.isExpired();

And note we specifically check if we are a browser in this case:

activemq/activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageConsumer.java

Line 1423 in e45ee4a

if (this.info.isBrowser() || md.getDeliverySequenceId() != 0l || !session.connection.isDuplicate(this, md.getMessage())) {

Is the actual problem that browsing is incrementing the counter when dispatched to a browser and shouldn't be?

[cshannon]

so re-reading what you wrote, you said it applies to a DLQ which would be an edge case but your change seems like it changes the behavior for every consumer and every queue and not just DLQs which we would not want.

If the goal is to simply be able to browse the DLQ and preventing not being able to read messages that were previously exceeding the redelivery policy then for your DLQ consumer can you simply disable the redelivery policy? Seems like a simple solution.

[mattrpav]

@cshannon the updated test has the full repro scenario.

commit: a7df4ca

The issue comes down to:

1. Should a browser send a poison ack if the message reaches max redelivery count and treat the browse as a delivery?

Currently, ActiveMQMessageConsumer does send a poison ack. The problem with that is it creates two messages, as the original stays on the source queue (dlq or otherwise) AND a message to a DLQ (or .DLQ.DLQ when browsing a .DLQ).

My argument is that browser should not send a poison ack if the max redelivery count is reached.

Note: This only occurs if QueueBrowser is created off of a transacted session.

[cshannon]

If you don't want to create the poison ack then couldn't you simply disable the redelivery policy when browsing the DLQ since it's an edge case? This seems like a client side config issue because you are setting that policy....so just don't set it when browsing the DLQ.

Consumers (including browsers) are designed to process the messages and check things like the redelivery counter or even expiration. A browser should not increment the redelivery counter, but I don't see how checking that counter and sending a poison ack is any different than checking expiration and expiring a message which it will do.

[mattrpav]

BUG: This blocks indefinitely

Scenario:

1. Message on the queue at the edge of maximumRedeliveries
2. Browse in transacted mode
3. Consumer code poison-acks the one message
4. Browse thinks there are messages to enumerate over, but there are not
5. nextElement blocks indefinitely waiting to get messages b/c no conditions are met to break out of the while(true) loop

[mattrpav]

Planned fix path forward:

1. Apply a new redeliveryPolicy option: queueBrowserIgnored = false (default behavior today)
2. In 7.x set queueBrowserIgnored = true (more consistent behavior for out-of-the-box JMS browse tools)

[mattrpav]

Is the actual problem that browsing is incrementing the counter when dispatched to a browser and shouldn't be?

Yes! With the caveat that it is only when browsing in a transacted session. In AUTO_ACK mode this behavior does not occur.