[ 
https://issues.apache.org/jira/browse/AMQ-6896?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Radek Kraus updated AMQ-6896:
-----------------------------
    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|https://issues.apache.org/jira/browse/AMQ-5785?focusedCommentId=15607861&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15607861].
 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).

  was:
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).


> 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
>            Priority: Minor
>         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 
> [comment|https://issues.apache.org/jira/browse/AMQ-5785?focusedCommentId=15607861&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15607861].
>  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)

Reply via email to