cshannon commented on code in PR #1551:
URL: https://github.com/apache/activemq/pull/1551#discussion_r2565749074
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/cursors/FilePendingMessageCursor.java:
##########
@@ -220,7 +232,7 @@ public synchronized boolean
tryAddMessageLast(MessageReference node, long maxWai
}
if (!hasSpace()) {
if (isDiskListEmpty()) {
- expireOldMessages();
+ expiredMessages.addAll(expireOldMessages());
Review Comment:
Same comment as in addMessageFirst
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/cursors/FilePendingMessageCursor.java:
##########
@@ -263,16 +284,16 @@ public synchronized void addMessageFirst(MessageReference
node) {
memoryList.addMessageFirst(node);
node.incrementReferenceCount();
setCacheEnabled(true);
- return;
+ return expiredMessages;
}
}
if (!hasSpace()) {
if (isDiskListEmpty()) {
- expireOldMessages();
+ expiredMessages = expireOldMessages();
Review Comment:
So I just realized that this is actually could be issue because the
expireOldMessages() method is going to decrement the reference on each message,
but the messages get copied to that temporary list. The problem is that now
that we are hanging onto the list reference (to process later outside of the
lock) we are more likely to blow memory because the hasSpace() method will
think there is free space but those messages are still held in the heap until
later. We are only adding one message but if that message happens to be very
large it could be an issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
