activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Bain (JIRA)" <j...@apache.org>
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
                 URL: https://issues.apache.org/jira/browse/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 (https://git-wip-us.apache.org/repos/asf?p=activemq.git;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
intent.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message