Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-5099 fix impossible cast in AMQPLargeMessage #5448

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ARTEMIS-5099 fix impossible cast in AMQPLargeMessage #5448

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions ...ocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.apache.activemq.artemis.core.persistence.Persister;
import org.apache.activemq.artemis.core.persistence.StorageManager;
import org.apache.activemq.artemis.core.persistence.impl.journal.LargeBody;
import org.apache.activemq.artemis.core.persistence.impl.journal.LargeServerMessageImpl;
import org.apache.activemq.artemis.core.server.ActiveMQServerLogger;
import org.apache.activemq.artemis.core.server.LargeServerMessage;
import org.apache.activemq.artemis.core.server.MessageReference;
Expand Down Expand Up @@ -660,7 +659,7 @@ public void referenceOriginalMessage(final Message original, final SimpleString

super.referenceOriginalMessage(original, originalQueue);

if (original instanceof LargeServerMessageImpl) {
if (original instanceof AMQPLargeMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main question would be what is the impact of adding this call into the mix given that it wasn't happening before and I guess didn't cause anything to break? Would something break now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say, "it wasn't happening before," what exactly do you mean? Do you mean that org.apache.activemq.artemis.core.persistence.impl.journal.LargeBody#referenceOriginalMessage wasn't being invoked since this instanceof would never succeed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what else I could have meant but let me be more clear. What will happen now that an AMQPLargeMessage cast would succeed when this method is called? Should something have been testing this? From a brief look at the broker code calling this it seems like the source message would never be anything other than the AMQPLargeMessage itself so I doubt the cast to ServerLargeMessageImpl would have succeeded in the past.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is supposed to be kept as it was...

it's being tested from TestConversions.. so it is for conversions...

So when you convert from AMQP to Core

It probably needs to add coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines break the existing code:

AMQPLargeMessage m = new AMQPLargeMessage(0, 0, null, null, null);
m.referenceOriginalMessage(new LargeServerMessageImpl((byte) 0, 0, null, null), SimpleString.of("foo"));

Causing this to be thrown:

java.lang.ClassCastException: class org.apache.activemq.artemis.core.persistence.impl.journal.LargeServerMessageImpl cannot be cast to class org.apache.activemq.artemis.protocol.amqp.broker.AMQPLargeMessage (org.apache.activemq.artemis.core.persistence.impl.journal.LargeServerMessageImpl and org.apache.activemq.artemis.protocol.amqp.broker.AMQPLargeMessage are in unnamed module of loader 'app')

It's not clear to me that this code is directly related to any conversion. If it was I would expect org.apache.activemq.artemis.core.persistence.impl.journal.LargeServerMessageImpl#referenceOriginalMessage to behave in a similar way.

this.largeBody.referenceOriginalMessage(((AMQPLargeMessage) original).largeBody);
}
}
Expand Down