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