activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Bain (JIRA)" <>
Subject [jira] [Created] (AMQ-5474) Broken ConsumerIdKey comparator implementation
Date Wed, 03 Dec 2014 19:33:12 GMT
Tim Bain created AMQ-5474:

             Summary: Broken ConsumerIdKey comparator implementation
                 Key: AMQ-5474
             Project: ActiveMQ
          Issue Type: Bug
          Components: Broker
    Affects Versions: 5.9.0
            Reporter: Tim Bain

One of the changes Gary made under the second batch of changes for AMQ-2327 (;a=commit;h=6c5732bc)
involved creating a comparator for AdvisoryBroker.ConsumerIdKey.  This comparator is broken
in one way, and inconsistent with ConsumerIdKey.equals() and .hashcode() in two others.  I'm
still using 5.8.0 so I can't say whether these problems cause the fix for AMQ-2327 to not
work in all cases, but that would be my concern.

Most significantly, if a and b have the same delegate but a's creationTime is before b's,
then comparator.compareTo(a, b) == -1 but comparator.compareTo(b, a) == 0.  This is flat-out
broken, and will probably cause incorrect sorting in the ConcurrentSkipListMap that was put
in place to fix AMQ-2327.

Next, if the creationTimes are equal, the delegates are compared.  But this comparison is
done via object equality (==) while ConsumerIdKey.equals() calls the delegate's equals() method.
 Presumably only one of these approaches is the right one and it should be used both places;
I'm guessing equals() is the way to go, though I could be wrong.

Finally, the comparator is not consistent with equals because there are objects for which
a.equals(b) but comparator.compareTo(a, b) != 0.  This might be OK, but I'd encourage another
look at both the comparator and ConsumerIdKey.equals() to make sure that that's really the

This message was sent by Atlassian JIRA

View raw message