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] [Commented] (AMQ-5474) Broken ConsumerIdKey comparator implementation
Date Wed, 03 Dec 2014 20:44:12 GMT

    [ https://issues.apache.org/jira/browse/AMQ-5474?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14233498#comment-14233498
] 

Tim Bain commented on AMQ-5474:
-------------------------------

I should have checked to be sure that code was still in 5.10.0 instead of relying solely on
the diffs from Gary's commit.  Thanks.

> 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)

Mime
View raw message