Qpid Proton
  1. Qpid Proton
  2. PROTON-474

Incorrect mapping of TTL to JMSExpiration in JMS InboundTransformer

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.5
    • Fix Version/s: 0.8
    • Component/s: proton-j
    • Labels:
      None

      Description

      The inbound message transformation of AMQP message to JMS message incorrectly sets the JMSExpiration to message TTL which is defined in milliseconds while the JMSExpiration value is a sum of the Message Timestamp and the producers TTL. The transformation should probably map the message creation time + the TTL to the setJMSExpiration method.

      1. JMSExpiration-patch.txt
        0.7 kB
        Timothy Bish
      2. PROTON-474-v2.patch
        4 kB
        Hiram Chirino
      3. PROTON-474-v3.patch
        4 kB
        Robbie Gemmell

        Issue Links

          Activity

          Hide
          Timothy Bish added a comment -

          Proposed patch for this issue.

          Show
          Timothy Bish added a comment - Proposed patch for this issue.
          Hide
          Hiram Chirino added a comment -

          Looks good to me.

          Show
          Hiram Chirino added a comment - Looks good to me.
          Hide
          Hiram Chirino added a comment -

          Attaching updated patch that also does a similar mapping on the outbound side.

          Show
          Hiram Chirino added a comment - Attaching updated patch that also does a similar mapping on the outbound side.
          Hide
          Hiram Chirino added a comment -

          Linking to an AMQ issue that depends on the patch.

          Show
          Hiram Chirino added a comment - Linking to an AMQ issue that depends on the patch.
          Hide
          Robbie Gemmell added a comment -

          JMSExpiration is annoying. Since the ttl field of the Header is mutable (where intermediaries should reduce it) and creation-time is an immutable field of the Properties, using creation-time + ttl as may give a JMSExpiration value earlier than intended.

          For the JMS Mapping currently being worked on in the OASIS AMQP Bindings & Mapping TC, the mapping for JMSExpiration is currently such that:

          • Clients will set both the absolute-expiry-time (used for the JMSExpiration value locally) and ttl fields on outbound messages if TTL is specified for the send/producer, so that receiving intermediaries can use which of the fields they like.
          • Messages arriving at the client map JMSExpiration from the immutable absolute-expiry-time if it set, or alternatively use current time + ttl header if that is set instead. The latter handles the case where creation-time wasnt specified, but also has potential to be fuzzy (this time, later than the desired expiration if any intermediaries havent adjusted the ttl header).
          Show
          Robbie Gemmell added a comment - JMSExpiration is annoying. Since the ttl field of the Header is mutable (where intermediaries should reduce it) and creation-time is an immutable field of the Properties, using creation-time + ttl as may give a JMSExpiration value earlier than intended. For the JMS Mapping currently being worked on in the OASIS AMQP Bindings & Mapping TC, the mapping for JMSExpiration is currently such that: Clients will set both the absolute-expiry-time (used for the JMSExpiration value locally) and ttl fields on outbound messages if TTL is specified for the send/producer, so that receiving intermediaries can use which of the fields they like. Messages arriving at the client map JMSExpiration from the immutable absolute-expiry-time if it set, or alternatively use current time + ttl header if that is set instead. The latter handles the case where creation-time wasnt specified, but also has potential to be fuzzy (this time, later than the desired expiration if any intermediaries havent adjusted the ttl header).
          Hide
          Hiram Chirino added a comment -

          Attaching new patch that incorporates Robbie Gemmell's comments.

          Show
          Hiram Chirino added a comment - Attaching new patch that incorporates Robbie Gemmell's comments.
          Hide
          Robbie Gemmell added a comment -

          In this bit of the outbound changes:

          +        long now = msg.getJMSTimestamp();
                   if( msg.getJMSExpiration() != 0 ) {
          +            if( now == 0 ) {
          +                now = System.currentTimeMillis();
          +            }
                       props.setAbsoluteExpiryTime(new Date(msg.getJMSExpiration()));
          +            header.setTtl(new UnsignedInteger((int)(msg.getJMSExpiration() - now)));
                   }
          

          Where 'now'=JMSTimestamp, it seems that it could set the ttl larger than it should be and additionally make it unrelated to the absoluteExpiryTime value which was just set. Where 'now' = System.currentTimeMillis(), then it seems possible the calculation could result in a negative, leading to some fun with later the UnsignedInteger.

          Show
          Robbie Gemmell added a comment - In this bit of the outbound changes: + long now = msg.getJMSTimestamp(); if( msg.getJMSExpiration() != 0 ) { + if( now == 0 ) { + now = System.currentTimeMillis(); + } props.setAbsoluteExpiryTime(new Date(msg.getJMSExpiration())); + header.setTtl(new UnsignedInteger((int)(msg.getJMSExpiration() - now))); } Where 'now'=JMSTimestamp, it seems that it could set the ttl larger than it should be and additionally make it unrelated to the absoluteExpiryTime value which was just set. Where 'now' = System.currentTimeMillis(), then it seems possible the calculation could result in a negative, leading to some fun with later the UnsignedInteger.
          Hide
          Hiram Chirino added a comment -

          Any thoughts how to resolve those outbound problems? Seems to me that (msg.getJMSExpiration()-System.currentTimeMillis()) is the most accurate TTL. Message could have already expired so what do we set the TTL to then? Perhaps we should exception out?

          Show
          Hiram Chirino added a comment - Any thoughts how to resolve those outbound problems? Seems to me that (msg.getJMSExpiration()-System.currentTimeMillis()) is the most accurate TTL. Message could have already expired so what do we set the TTL to then? Perhaps we should exception out?
          Hide
          Robbie Gemmell added a comment -

          'JMSExpiration - current' is what I would do (assuming JMSExpiration is set of course). If that was <=0 then I guess the choice is setting it to 0/1 or try not to send the message, possibly by throwing an exception as you suggest.

          (Curse the new JIRA phone interface which doesn't let you select and move the cursor properly)

          Show
          Robbie Gemmell added a comment - 'JMSExpiration - current' is what I would do (assuming JMSExpiration is set of course). If that was <=0 then I guess the choice is setting it to 0/1 or try not to send the message, possibly by throwing an exception as you suggest. (Curse the new JIRA phone interface which doesn't let you select and move the cursor properly)
          Hide
          Robbie Gemmell added a comment - - edited

          Attaching PROTON-474-v3.patch

          Changes to InboundTransformer are as previously, changes to JMSMappingOutboundTransformer updated based on above discussion. If JMSExpiration is not 0, absolute-expiry-time is set to match, and TTL is set to <current> - JMSExpiration or 1 if that proves negative due to the possible race since deciding to send the message.

          Show
          Robbie Gemmell added a comment - - edited Attaching PROTON-474 -v3.patch Changes to InboundTransformer are as previously, changes to JMSMappingOutboundTransformer updated based on above discussion. If JMSExpiration is not 0, absolute-expiry-time is set to match, and TTL is set to <current> - JMSExpiration or 1 if that proves negative due to the possible race since deciding to send the message.
          Hide
          ASF subversion and git services added a comment -

          Commit 1607394 from Robbie Gemmell in branch 'proton/trunk'
          [ https://svn.apache.org/r1607394 ]

          PROTON-474: update mapping of JMSExpiration <-> absolute-expiry-time/ttl in contrib/proton-jms transformers

          Show
          ASF subversion and git services added a comment - Commit 1607394 from Robbie Gemmell in branch 'proton/trunk' [ https://svn.apache.org/r1607394 ] PROTON-474 : update mapping of JMSExpiration <-> absolute-expiry-time/ttl in contrib/proton-jms transformers

            People

            • Assignee:
              Robbie Gemmell
              Reporter:
              Timothy Bish
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development