[
https://issues.apache.org/jira/browse/QPID-2418?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12850103#action_12850103
]
Robbie Gemmell commented on QPID-2418:
--------------------------------------
The approach taken in the patch looks good, but I spotted some issues wih it
that need addressed, and an opportunity to get rid of a lot of [pre-existing]
duplication:
The unsubscribe method synchronises around the _subscriptions map, but nothing
else appears to. The content in the various maps within the block are
accessed/updated from a few other locations (in AMQsession, and its 0_8 and
0_10 subclasses), and the content in multiple maps is generally directly
related (eg subscriptions and reverse subscriptions maps, and the topics and
selectors maps) so there could still be threading issues despite the
synchorized block in unsubscribe. The various other uses of the maps should
also be protected to ensure the associated information in the multiple maps
they access/update is properly matched.
When a new durable subscription is made without a selector, it should also
check if any known existing/previous subscription using the same name DID have
a selector, and if so should both unsubscribe the subscription name _and_
remove the existing selector from the selectors map.
In AMQSession.createDurableSubscriber(Topic topic, String name, String
messageSelector, boolean noLocal), when removing an entry from the
reverseSubscriptionsMap the subscription is given as the key. It should
actually be the parent Consumer which is provided as the key. The same applies
in the unsubscribe method (existing bug).
When adding the selector to the subscriptionSelectors map the value is not
permitted to be null, but the selector value is not checked before adding it.
The value can quite easily be null here, as although it is retrieved from the
consumer object it is the same String that is passed into the create method,
meaning it can be provided null and is actually set to null in the method
itself if the selector string given 'isBlank'.
A small update was made in the AMQSession_0_8 and 0_10 subclasses. Examining
the almost-identical methods more closesly seems to show that although they
look to do slightly different things and call differnet methods to achieve
their result, they do infact ultimately end up doing exactly the same thing.
These methods (and those they call) should be rationalised and the
createDurableSubscriber(Topic, String) method implementation moved into the
abstract superclass with the comparible selector method variant which already
resides there (those 2 methods can then potentially be reduced to a common
implementation too).
> Existing durable subscription with selector is not unsubscribed during
> change to new subscription
> --------------------------------------------------------------------------------------------------
>
> Key: QPID-2418
> URL: https://issues.apache.org/jira/browse/QPID-2418
> Project: Qpid
> Issue Type: Bug
> Components: Java Client
> Affects Versions: M4, 0.5, 0.6
> Reporter: Robbie Gemmell
> Assignee: Robbie Gemmell
> Fix For: 0.7
>
> Attachments:
> 0001-QPID-2418-Unsubscribe-existing-durable-subscription.patch
>
>
> AMQSession.createDurableSubscriber(topic, name, messageSelector, noLocal)
> does not unsubscribe existing durable subscriptions. Whilst it does check for
> existing durable subscriptions in use by the client with the same name, it
> instead simply closes the subscriptions then creates a new one. As a result
> of not unsubscribing, the queue backing the subscription is not deleted
> before being used by the updated subscription as it should be (and as happens
> in the 0_8 and 0_10 subclasses when using durable subscriptions without
> selectors).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org
