git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[GitHub] activemq-artemis pull request #2115: ARTEMIS-1858 Expiry messages are not tr...


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.


---