[jira] [Updated] (AMQ-5474) Broken ConsumerIdKey comparator implementation

["Tim Bain (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/AMQ-5474?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Tim Bain updated AMQ-5474:
--------------------------
    Description: 
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.

  was:
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.


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