Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2115#discussion_r191777782
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
---
@@ -606,22 +606,36 @@ public String getAddress() {
@Override
public AMQPMessage setAddress(String address) {
- this.address = SimpleString.toSimpleString(address,
coreMessageObjectPools == null ? null :
coreMessageObjectPools.getAddressStringSimpleStringPool());
+ internalSetAddress(address);
+ setProtonAddress(address);
return this;
}
+ private void internalSetAddress(String address) {
+ this.address = SimpleString.toSimpleString(address,
coreMessageObjectPools == null ? null :
coreMessageObjectPools.getAddressStringSimpleStringPool());
+ }
+
@Override
public AMQPMessage setAddress(SimpleString address) {
this.address = address;
+ setProtonAddress(address.toString());
--- End diff --
> this is used on expiry or DLQ. you could argue those are copied messages
and new messages.
I can see that view can be argued, though I dont personally take that view
myself. Thats not really the main thing that concerned me.
> I had the option to use extraProperties which only transverses through
the broker... however there are implications on protocol conversions that could
open further possibilities for bugs. this was the simpler change given it's
copying the message.
>
> Notice that setAddress will not be effective unless the message is
reencoded.. so it only works on Expiry or DLQ.
I think this opens cases for other general bugs and protocol violations
too. Having since discussed a bit with @mtaylor and looked myself, the
setAddress methods (there are two, you only added javadoc for the lesser used
one) seem to be used in a number of important places, which is what I was
concerned about given the new side effect it has. I hadn't twigged on the need
to explicitly reencode, but in general think its a bad idea for the in-memory
representation of the message to generally differ from the sent payload as it
seems like it may now in many cases. That also means when anyone does do
something and reencode (e.g in a divert) they may get side effects they had no
part in.
---
