Qpid
  1. Qpid
  2. QPID-2418

Existing durable subscription with selector is not unsubscribed during change to new subscription

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: M4, 0.5, 0.6
    • Fix Version/s: 0.7
    • Component/s: Java Client
    • Labels:
      None

      Description

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

      1. QPID-2418-trunk_robbies_additions.patch
        21 kB
        Robbie Gemmell
      2. QPID-2418-trunk_new.patch
        50 kB
        Robbie Gemmell
      3. 0001-QPID-2418-trunk.patch
        53 kB
        Andrew Kennedy
      4. 0001-QPID-2418-branch.patch
        51 kB
        Andrew Kennedy

        Activity

        Hide
        Marnie McCormack added a comment -

        Patches reviewed/applied by RGe.

        Show
        Marnie McCormack added a comment - Patches reviewed/applied by RGe.
        Hide
        Robbie Gemmell added a comment -

        As per Andrews earlier comments, the updates do not fully resolve the issue when using 0-8/0-9/0-9-1 clients as the information on previous subscribers is only held per-session, and regardless of this it is not possible with these protocol versions to determine when connecting whether any existing server side selector (if any) matches the new one (if any) as the arguments can not be queried. It is only possible to tell from the queue bindings whether the Topic in use differs differs.

        The updates I made to augment the original patch do allow the 0-10 client to determine that the selector is changing even without knowledge of any prior subscribers on the session and so should fully resolve the issue. This was achieved by always sending the selector argument but giving it an empty value if no selector is in use (the broker interprets the empty argument as lack of a selector just as it would if the argument was not present), as this allows querying the broker upon (re)connection whether the selector argument matches any existing argument. Additionally, the client side selectors in the 0-10 client would already protect from reciept of messages which do not match the selector in use at any given point.

        Show
        Robbie Gemmell added a comment - As per Andrews earlier comments, the updates do not fully resolve the issue when using 0-8/0-9/0-9-1 clients as the information on previous subscribers is only held per-session, and regardless of this it is not possible with these protocol versions to determine when connecting whether any existing server side selector (if any) matches the new one (if any) as the arguments can not be queried. It is only possible to tell from the queue bindings whether the Topic in use differs differs. The updates I made to augment the original patch do allow the 0-10 client to determine that the selector is changing even without knowledge of any prior subscribers on the session and so should fully resolve the issue. This was achieved by always sending the selector argument but giving it an empty value if no selector is in use (the broker interprets the empty argument as lack of a selector just as it would if the argument was not present), as this allows querying the broker upon (re)connection whether the selector argument matches any existing argument. Additionally, the client side selectors in the 0-10 client would already protect from reciept of messages which do not match the selector in use at any given point.
        Hide
        Robbie Gemmell added a comment -

        Original patch updated (QPID-2418-trunk_new.patch) and applied to trunk, and modifications added to enable the tests to pass on all profiles.

        Show
        Robbie Gemmell added a comment - Original patch updated ( QPID-2418 -trunk_new.patch) and applied to trunk, and modifications added to enable the tests to pass on all profiles.
        Hide
        Robbie Gemmell added a comment -

        Attaching QPID-2418-trunk_robbies_additions.patch, that when paired with the 'original' QPID-2418-trunk_new.patch enables all the tests to pass.

        (This was done to allow properly attributing the changes and to keep the updates separate to ease review for anyone else, as I have already reviewed the significant original patch)

        Show
        Robbie Gemmell added a comment - Attaching QPID-2418 -trunk_robbies_additions.patch, that when paired with the 'original' QPID-2418 -trunk_new.patch enables all the tests to pass. (This was done to allow properly attributing the changes and to keep the updates separate to ease review for anyone else, as I have already reviewed the significant original patch)
        Hide
        Robbie Gemmell added a comment -

        Adding a further updated version of the patch, QPID-2418-trunk_new3.patch

        Streamlines some of the previous additions and fixes one of the test failures on the Java 0-10 profiles, which seemed to relate to the asynchronous batched acks performed by the 0-10 client in AUTO_ACK mode as it appeared that the messages hadnt been acked when the queue count was checked after consumption; switching to a transacted session alleviated the issue.

        There is 1 remaining failure on the Java 0-10 profiles still requiring investigation (DurableSubscriptionTest#testResubscribeWithChangedSelector, where it seems a message is received which should not be).

        Show
        Robbie Gemmell added a comment - Adding a further updated version of the patch, QPID-2418 -trunk_new3.patch Streamlines some of the previous additions and fixes one of the test failures on the Java 0-10 profiles, which seemed to relate to the asynchronous batched acks performed by the 0-10 client in AUTO_ACK mode as it appeared that the messages hadnt been acked when the queue count was checked after consumption; switching to a transacted session alleviated the issue. There is 1 remaining failure on the Java 0-10 profiles still requiring investigation (DurableSubscriptionTest#testResubscribeWithChangedSelector, where it seems a message is received which should not be).
        Hide
        Robbie Gemmell added a comment -

        Adding a further updated version of the patch, QPID-2418-trunk_new2.patch

        These additional updates allow the new/updated tests to pass when run using either the C++ broker profiles (with the tests removed from the relevant excludes file), or the Java 0-8/0-9 broker profiles. The Java 0-10 profiles still fail some of the tests and require further investigation.

        Show
        Robbie Gemmell added a comment - Adding a further updated version of the patch, QPID-2418 -trunk_new2.patch These additional updates allow the new/updated tests to pass when run using either the C++ broker profiles (with the tests removed from the relevant excludes file), or the Java 0-8/0-9 broker profiles. The Java 0-10 profiles still fail some of the tests and require further investigation.
        Hide
        Robbie Gemmell added a comment -

        Adding an updated version of the original patch, named QPID-2418-trunk_new.patch

        The only changes made were to update import statements and add any modifications required to address code updates since the original patch was created, in order that it will apply and compile. Further modifications are still required to address testing issues with the C++ and 0-10 Java profiles.

        Show
        Robbie Gemmell added a comment - Adding an updated version of the original patch, named QPID-2418 -trunk_new.patch The only changes made were to update import statements and add any modifications required to address code updates since the original patch was created, in order that it will apply and compile. Further modifications are still required to address testing issues with the C++ and 0-10 Java profiles.
        Hide
        Robbie Gemmell added a comment -

        The patch has definitely rotted a bit and needs updating, plus i think the tests still need updated to account for the difference in queue behaviour between brokers when selectors are used (and also, the 0-10 java client not currently sending the server side selector argument to the Java broker too I think ), but Andrew or myself will get to this some time soon.

        Robbie.

        Show
        Robbie Gemmell added a comment - The patch has definitely rotted a bit and needs updating, plus i think the tests still need updated to account for the difference in queue behaviour between brokers when selectors are used (and also, the 0-10 java client not currently sending the server side selector argument to the Java broker too I think ), but Andrew or myself will get to this some time soon. Robbie.
        Hide
        Rajith Attapattu added a comment -

        Robbie,

        I believe you put this on hold based on a request from me.
        Sorry for not following up with you on this.

        Would you like to go ahead and commit this patch now?

        Regards,

        Rajith

        Show
        Rajith Attapattu added a comment - Robbie, I believe you put this on hold based on a request from me. Sorry for not following up with you on this. Would you like to go ahead and commit this patch now? Regards, Rajith
        Hide
        Robbie Gemmell added a comment -

        At some point next week.

        Show
        Robbie Gemmell added a comment - At some point next week.
        Hide
        Rajith Attapattu added a comment -

        Robbie,

        When are you planning to apply the trunk patch?

        Rajith

        Show
        Rajith Attapattu added a comment - Robbie, When are you planning to apply the trunk patch? Rajith
        Hide
        Robbie Gemmell added a comment -

        Applied patch to the branch only.

        Trunk patch still needs to be reviewed and applied.

        Show
        Robbie Gemmell added a comment - Applied patch to the branch only. Trunk patch still needs to be reviewed and applied.
        Hide
        Andrew Kennedy added a comment -

        Revised patches. These do not fix this issue completely, since the information stored regarding subscriptions on the client is per-session, and should really be per-connection for behaviour to be consistent. In order not to introduce such inconsistent behaviour, I have changed the code to throw a JMS IllegalStateException on any attempt to re-create a subscription (with the same paramaters) that already exists on the session, and unsubscribe if the re-created subscription is a change to an existing open subscription on the session, otherwise behaviour is unchanged. The code adds locking of the subscription information and gets rid of duplicated code across protocol versions, but a complete fix for this issue will require more extensive changes.

        Show
        Andrew Kennedy added a comment - Revised patches. These do not fix this issue completely, since the information stored regarding subscriptions on the client is per-session, and should really be per-connection for behaviour to be consistent. In order not to introduce such inconsistent behaviour, I have changed the code to throw a JMS IllegalStateException on any attempt to re-create a subscription (with the same paramaters) that already exists on the session, and unsubscribe if the re-created subscription is a change to an existing open subscription on the session, otherwise behaviour is unchanged. The code adds locking of the subscription information and gets rid of duplicated code across protocol versions, but a complete fix for this issue will require more extensive changes.
        Hide
        Rajith Attapattu added a comment -

        Interesting, the queue declare with the new selector merely updates the the selector on the queue?
        The AMQP spec doesn't really define the behaviour if a queue declare is sent with the same name but different args (in this case the selector).
        So I wouldn't argue that the Java broker is wrong. The c++ client merely ignores the queue declare.
        IMO I think both brokers are wrong as this can lead to subtle errors that can go undetected - but thats separate topic all together.

        So on the C++ side, the queue is not deleted due to the java client bug.
        So the message is there and gets sent to the client who now has a new selector (with 0-10 client side selectors) which matches and the message is delivered.

        Anyways once we fix the Java client to issue a queue this should be taken care of.
        Always a good idea to run the default java and c++ profiles before an update.

        Show
        Rajith Attapattu added a comment - Interesting, the queue declare with the new selector merely updates the the selector on the queue? The AMQP spec doesn't really define the behaviour if a queue declare is sent with the same name but different args (in this case the selector). So I wouldn't argue that the Java broker is wrong. The c++ client merely ignores the queue declare. IMO I think both brokers are wrong as this can lead to subtle errors that can go undetected - but thats separate topic all together. So on the C++ side, the queue is not deleted due to the java client bug. So the message is there and gets sent to the client who now has a new selector (with 0-10 client side selectors) which matches and the message is delivered. Anyways once we fix the Java client to issue a queue this should be taken care of. Always a good idea to run the default java and c++ profiles before an update.
        Hide
        Robbie Gemmell added a comment -

        The changes made to DurableSubscriptionTest were not intended to yet expose this bug any more than it was, and were deliberately kept short so that they wouldnt (the comment is innacurate youll note, as it isnt doing what it was originally and I forgot to update that). The new test added in DurableSubscriberTest that absolutely would expose it was excluded until it is fixed so that automated builds wouldnt be been broken. What I didnt do was run the test using the C++ broker as I didnt think I was changing anything of any particular note, but I failed to consider that it could behave slightly differently due to use of client side selectors rather than the server side selectors used by the Java broker. The broker doenst create a new queue, but it does change the filters used to select messages meaning that messges never get onto the queue which will on the C++ broker, which I imagine is where the test now fails. I'll take that into account when updating it.

        Show
        Robbie Gemmell added a comment - The changes made to DurableSubscriptionTest were not intended to yet expose this bug any more than it was, and were deliberately kept short so that they wouldnt (the comment is innacurate youll note, as it isnt doing what it was originally and I forgot to update that). The new test added in DurableSubscriberTest that absolutely would expose it was excluded until it is fixed so that automated builds wouldnt be been broken. What I didnt do was run the test using the C++ broker as I didnt think I was changing anything of any particular note, but I failed to consider that it could behave slightly differently due to use of client side selectors rather than the server side selectors used by the Java broker. The broker doenst create a new queue, but it does change the filters used to select messages meaning that messges never get onto the queue which will on the C++ broker, which I imagine is where the test now fails. I'll take that into account when updating it.
        Hide
        Rajith Attapattu added a comment -

        When investigating the test failure for "testResubscribeWithChangedSelector" in the DurableSubscriptionTest, I was a bit shocked to see the amount of code duplication.
        This may have been due to a merge, but we definitely need to fix this. So thx for taking care of this.

        I haven't had chance to have a closer look at the patches, but please make sure tests pass against both the C++ and Java Broker before committing it.

        Also the change made in rev 929095 to the DurableSubscriptionTest.java (method testResubscribeWithChangedSelector) is somehow passing for the Java broker.
        Bcos we have the bug mentioned in this bug, it should be failing in both brokers.
        I am not so happy about checking in the change to the test case before a fix is committed as this has broken our automated builds.

        Perhaps the Java broker is creating a new queue as the arguments list is different (the selector being different) ?
        Isn't that a bug? Perhaps worthwhile investigating this.

        Show
        Rajith Attapattu added a comment - When investigating the test failure for "testResubscribeWithChangedSelector" in the DurableSubscriptionTest, I was a bit shocked to see the amount of code duplication. This may have been due to a merge, but we definitely need to fix this. So thx for taking care of this. I haven't had chance to have a closer look at the patches, but please make sure tests pass against both the C++ and Java Broker before committing it. Also the change made in rev 929095 to the DurableSubscriptionTest.java (method testResubscribeWithChangedSelector) is somehow passing for the Java broker. Bcos we have the bug mentioned in this bug, it should be failing in both brokers. I am not so happy about checking in the change to the test case before a fix is committed as this has broken our automated builds. Perhaps the Java broker is creating a new queue as the arguments list is different (the selector being different) ? Isn't that a bug? Perhaps worthwhile investigating this.
        Hide
        Robbie Gemmell added a comment -

        On line 1086 (after patching) this code segment needs updated:
        if (_subscriptionTopics.containsKey(name) && _subscriptionSelectors.containsKey(name))
        {
        if (!_subscriptionTopics.get(name).equals(topic) || !_subscriptionSelectors.get(name).equals(messageSelector))

        { _logger.info("Unsubscribing from topic " + topic + " with subscription exchange " + name + " and selector " + messageSelector); unsubscribe(name); }

        }

        If the subscription name has been seen before but used without a selector, then in the outer if statement the first test will success but the second will fail, causing the inner if statement to detect subscription change not to be evaluated. However, if the new subscription with reused name happens to be adding a selector then this should be classified as a selector change (from no selector) and cause unsubscription before proceeding.

        Also, there is possibility of deadlock when creation of a new durable subscriber occurs with the same subscription name as an existing open subscriber, which will cause unsubscribe+closure of the old subscriber. The create method will hold the new lock added, but closing the subscription will contact the broker, which will reply and then a different thread will execute the returned BasicCancelOK method and try to aquire the lock in deregisterConsumer() in order to complete the closure, causing the first thread executing the close to timeout and throw an exception. Looking at the map usages it seems there is actually no need to acquire the same lock in deregisterConsumer(), only a need to govern access to the subscriptions and reverseSubscriptionMaps, which could be achieved using a second lock that doesnt encompass the subscriber.close() operation.

        Show
        Robbie Gemmell added a comment - On line 1086 (after patching) this code segment needs updated: if (_subscriptionTopics.containsKey(name) && _subscriptionSelectors.containsKey(name)) { if (!_subscriptionTopics.get(name).equals(topic) || !_subscriptionSelectors.get(name).equals(messageSelector)) { _logger.info("Unsubscribing from topic " + topic + " with subscription exchange " + name + " and selector " + messageSelector); unsubscribe(name); } } If the subscription name has been seen before but used without a selector, then in the outer if statement the first test will success but the second will fail, causing the inner if statement to detect subscription change not to be evaluated. However, if the new subscription with reused name happens to be adding a selector then this should be classified as a selector change (from no selector) and cause unsubscription before proceeding. Also, there is possibility of deadlock when creation of a new durable subscriber occurs with the same subscription name as an existing open subscriber, which will cause unsubscribe+closure of the old subscriber. The create method will hold the new lock added, but closing the subscription will contact the broker, which will reply and then a different thread will execute the returned BasicCancelOK method and try to aquire the lock in deregisterConsumer() in order to complete the closure, causing the first thread executing the close to timeout and throw an exception. Looking at the map usages it seems there is actually no need to acquire the same lock in deregisterConsumer(), only a need to govern access to the subscriptions and reverseSubscriptionMaps, which could be achieved using a second lock that doesnt encompass the subscriber.close() operation.
        Hide
        Andrew Kennedy added a comment -

        Updated patches for branch and trunk

        Show
        Andrew Kennedy added a comment - Updated patches for branch and trunk
        Hide
        Robbie Gemmell added a comment -

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

        Show
        Robbie Gemmell added a comment - 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).
        Hide
        Andrew Kennedy added a comment -

        Unsubscribe existing durable subscriptions on create when they change

        Show
        Andrew Kennedy added a comment - Unsubscribe existing durable subscriptions on create when they change

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Robbie Gemmell
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development