[
https://issues.apache.org/jira/browse/AMQ-5474?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Timothy Bish closed AMQ-5474.
-----------------------------
Resolution: Duplicate
This code has been fixed in later releases.
> 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 (consistency with equals isn't required, though it will result in
> behavior that's consistent but unexpected), but I'd encourage another look at
> both the comparator and ConsumerIdKey.equals() to make sure that that's
> really the intent. In particular, I'm concerned about whether
> consumers.remove(new ConsumerIdKey(info.getConsumerId())) will work properly
> if the comparator isn't consistent with equals, since for a SortedMap we're
> going to be using compareTo() and it doesn't consider two keys equal if they
> have the same delegate but different creationTimes. (That's the "consistent
> but unexpected" behavior I referenced earlier.) So the current behavior
> *might* be acceptable on this point, but I'm very skeptical and I'd want a
> unit test that proves that the put(), get(), and remove() operations really
> work as a developer would expect before accepting that it's valid for this
> comparator to be inconsistent with equals.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
