ActiveMQ
  1. ActiveMQ
  2. AMQ-3153

An expired message that is consumed and resent with an updated expiration never expires again.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.4.2
    • Fix Version/s: 5.5.0
    • Component/s: Broker
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      Symptom
      ========
      We have a use case where a producer sends a message to a queue with an expiration. When the message expires to the DLQ, a consumer on the DLQ receives the message, modifies it, and resends it to the original queue with an updated expiration.

      When the expired message is resent, it is given a new JMS message ID, so is, for all intents and purposes, a new message. However, althought the resent message has an updated expiration, it never expires to the DLQ.

      Cause
      =====
      When a message expires, an "originalExpiration" property is added to the message by RegionBroker.stampAsExpired:

      private boolean stampAsExpired(Message message) throws IOException {
      boolean stamped=false;
      if (message.getProperty(ORIGINAL_EXPIRATION) == null)

      { long expiration=message.getExpiration(); message.setProperty(ORIGINAL_EXPIRATION,new Long(expiration)); stamped = true; }

      return stamped;
      }

      When the consumer receives and resends the expired message, ActiveMQSession gives the message a new ID and updates its expiration:

      protected void send(ActiveMQMessageProducer producer, ActiveMQDestination destination, Message message, int deliveryMode, int priority, long timeToLive,
      MemoryUsage producerWindow, int sendTimeout) throws JMSException {
      ..
      long expiration = 0L;
      if (!producer.getDisableMessageTimestamp()) {
      long timeStamp = System.currentTimeMillis();
      message.setJMSTimestamp(timeStamp);
      if (timeToLive > 0)

      { expiration = timeToLive + timeStamp; }

      }
      message.setJMSExpiration(expiration);
      ...

      // Set the message id.
      if (msg == message)

      { msg.setMessageId(new MessageId(producer.getProducerInfo().getProducerId(), sequenceNumber)); }

      else

      { msg.setMessageId(new MessageId(producer.getProducerInfo().getProducerId(), sequenceNumber)); message.setJMSMessageID(msg.getMessageId().toString()); }

      ...

      At this point the resent message has a new ID and a new expiration, so it should be allowed to reexpire. However, the resent message still carries the originalExpiration property, which makes RegionBroker report the message has not expired (even though it may have):

      @Override
      public boolean isExpired(MessageReference messageReference) {
      boolean expired = false;
      if (messageReference.isExpired()) {
      try {
      // prevent duplicate expiry processing
      Message message = messageReference.getMessage();
      synchronized (message)

      { expired = stampAsExpired(message); }

      } catch (IOException e)

      { LOG.warn("unexpected exception on message expiry determination for: " + messageReference, e); }

      }
      return expired;
      }

      Since the broker is not reporting the message as expired, the expired message processing in Queue bypasses the message (from Queue.doBrowse()):

      if (broker.isExpired(node))

      { LOG.debug("expiring from messages: " + node); messageExpired(connectionContext, createMessageReference(node.getMessage())); }

      messages.remove();

      Solution
      =======
      Whenever a message is sent to a broker from a message producer, it should have its originalExpiration property cleared. I've provided a patch in ActiveMQSession to do this, but I'm not familiar enough with the workflow to know if this is the appropriate place — I'm specifically unhappy with the need to case the javax.jms.Message to an ActiveMQMessage in order to clear the readonly properties.

      1. AMQ3513.patch
        6 kB
        Stirling Chow

        Activity

        Hide
        Stirling Chow added a comment -

        Patch that clears the originalExpiration property when a message is re-sent and a unit test that fails with AMQ 5.4.2 and passes once the patch is applied.

        Show
        Stirling Chow added a comment - Patch that clears the originalExpiration property when a message is re-sent and a unit test that fails with AMQ 5.4.2 and passes once the patch is applied.
        Hide
        Gary Tully added a comment -

        patch looks good and it is great to have a testcase

        One thought, I wonder if it makes sense to have the originalExpiration property omitted from a message copy. There is by default a copy of a message before sending by a session.
        So I am thinking it may be better to remove that property towards the end of:
        org.apache.activemq.command.Message#copy

        In org.apache.activemq.broker.region.RegionBroker#sendToDeadLetterQueue the message is stamped after a copy and there is no further copy when it is sent to the DQL.

        It may be worth a try.

        Show
        Gary Tully added a comment - patch looks good and it is great to have a testcase One thought, I wonder if it makes sense to have the originalExpiration property omitted from a message copy. There is by default a copy of a message before sending by a session. So I am thinking it may be better to remove that property towards the end of: org.apache.activemq.command.Message#copy In org.apache.activemq.broker.region.RegionBroker#sendToDeadLetterQueue the message is stamped after a copy and there is no further copy when it is sent to the DQL. It may be worth a try.
        Hide
        Timothy Bish added a comment -

        Agree with Gary on this one, the Message class copy method is a good place to remove the original expiration property as it really only makes sense when message is on the DLQ. I've added the supplied unit test along with that fix, tests passing fine now.

        Show
        Timothy Bish added a comment - Agree with Gary on this one, the Message class copy method is a good place to remove the original expiration property as it really only makes sense when message is on the DLQ. I've added the supplied unit test along with that fix, tests passing fine now.
        Hide
        Timothy Bish added a comment -

        Fix in trunk, test added, thanks.

        Show
        Timothy Bish added a comment - Fix in trunk, test added, thanks.

          People

          • Assignee:
            Timothy Bish
            Reporter:
            Stirling Chow
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development