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

["Radek Kraus (JIRA)" <ji...@apache.org>]

Radek Kraus created AMQ-6896:
--------------------------------

             Summary: FilePendingMessageCursor - Invalid destination statistics (message expiration)
                 Key: AMQ-6896
                 URL: https://issues.apache.org/jira/browse/AMQ-6896
             Project: ActiveMQ
          Issue Type: Bug
          Components: Broker
    Affects Versions: 5.15.2, 5.15.3
            Reporter: Radek Kraus
         Attachments: FilePendingMessageCursor.patch, MemoryMessageExpirationDestStatsTest.java

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 AMQ-5785 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).



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)