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)
