[AMQ-6896] FilePendingMessageCursor - Invalid destination statistics (message expiration)

• Type: Bug
• Status: Open
• Priority: Minor
• Resolution: Unresolved
• Affects Version/s: 5.15.2, 5.15.3
• Component/s: Broker
• Assignee: Unassigned
• Reporter: Radek Kraus
• Created: 08/Feb/18 14:42
• Updated: 08/Feb/18 16:35

Description

I found a situation, where destination statistics (QueueSize, ExpiredCount, DequeueCount) shows wrong values.

This problem was caused by AMQ-5785 fix (deadlock in FilePendingMessageCursor between "incoming send operations" and "onUsageChanged" event), see comment. The expireOldMessages() method originally invokes discardExpiredMessage(MessageReference) method. The fix (done by AMQ-5785) "moved" invocation of discardExpiredMessage(MessageReference) method out of the synchronized section (deadlock was solved). Unfortunately this change was done only in onUsageChanged(Usage usage, int, int) method. But other methods (tryAddMessageLast(MessageReference, long) and addMessageFirst(MessageReference)), which invokes expireOldMessages() method too, were not changed, what causes that discardExpiredMessage(MessageReference method is not invoked at all => destination statistics is not updated finally.

Let suppose following test case (situation):

• Thread 1 invokes tryAddMessageLast method
  • there is no space and expireOldMessages method is invoked
  • list with expired messages is returned (expired messages are removed from memoryList)
  • but the returned list is simply ignored (no invocation of discardExpiredMessage(MessageReference method)
• Thread 1 finish invocation of tryAddMessageLast method and synchronization lock is released
• now the "delayed" invocation of UsageListener.onUsageChanged method is invoked (thread from ThreadPoolExecutor, Usage class)
  • usually this invocation is done first and discardExpiredMessage(MessageReference) method is invoked
  • but when this invocation comes later, no expired messages is found (because it was already removed by Thread 1) and discardExpiredMessage(MessageReference) method is not invoked at all

I think that expired message list must not be ignored in tryAddMessageLast and addMessageFirst methods and "discard" operation must be invoked too (like is already done in onUsageChanged method).

I attached the test case, where the situation is simulated by "init delay thread pool executor" (init delay before the task is started). Additionally I attached fix proposal, where I add invocation of "discard" method (outside of synchronization section) into tryAddMessageLast and addMessageFirst methods too (instead of ignoring returned expired message list).