QPID-3064, QPID-3157: ensure that if the node marker is pointing at the tail node when it is removed, the marker is still subsequently able to find new subscriptions at the end of the list

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1152633 13f79535-47bb-0310-9956-ffa450edef68

2 files changed, 38 insertions, 2 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/subscription/SubscriptionList.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/subscription/SubscriptionList.java
index 6dabfd21e2..3e6299cb8a 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/subscription/SubscriptionList.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/subscription/SubscriptionList.java
@@ -191,7 +191,7 @@ public class SubscriptionList
         return false;
     }
 
-    private void nodeMarkerCleanup(SubscriptionNode node)
+    private void nodeMarkerCleanup(final SubscriptionNode node)
     {
         SubscriptionNode markedNode = _subNodeMarker.get();
         if(node == markedNode)
@@ -200,8 +200,10 @@ public class SubscriptionList
             //replace it with a dummy pointing at the next node.
             //this is OK as the marked node is only used to index
             //into the list and find the next node to use.
+            //Because we inserted a dummy if node was the
+            //tail, markedNode.nextNode() can never be null.
             SubscriptionNode dummy = new SubscriptionNode();
-            dummy.setNext(markedNode.findNext());
+            dummy.setNext(markedNode.nextNode());
 
             //if the CAS fails the marked node has changed, thus
             //we don't care about the dummy and just forget it
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/subscription/SubscriptionListTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/subscription/SubscriptionListTest.java
index a9acfccd8e..c4d1a1e614 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/subscription/SubscriptionListTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/subscription/SubscriptionListTest.java
@@ -303,6 +303,40 @@ public class SubscriptionListTest extends QpidTestCase
     }
 
     /**
+     * Test that the marked node 'findNext' behaviour is as expected after a subscription is added
+     * to the list following the tail subscription node being removed while it is the marked node.
+     * That is, that the new subscriptions node is returned by getMarkedNode().findNext().
+     */
+    public void testMarkedNodeFindsNewSubscriptionAfterRemovingTailWhilstMarked()
+    {
+        //get the node out the list for the 3rd subscription
+        SubscriptionNode sub3Node = getNodeForSubscription(_subList, _sub3);
+        assertNotNull("Should have been a node present for the subscription", sub3Node);
+
+        //mark the 3rd subscription node
+        assertTrue("should have succeeded in updating the marked node",
+                _subList.updateMarkedNode(_subList.getMarkedNode(), sub3Node));
+
+        //verify calling findNext on the marked node returns null, i.e. the end of the list has been reached
+        assertEquals("Unexpected node after marked node", null, _subList.getMarkedNode().findNext());
+
+        //remove the 3rd(marked) subscription from the list
+        assertTrue("Removing subscription node should have succeeded", _subList.remove(_sub3));
+
+        //add a new 4th subscription to the list
+        Subscription sub4 = new MockSubscription();
+        _subList.add(sub4);
+
+        //get the node out the list for the 4th subscription
+        SubscriptionNode sub4Node = getNodeForSubscription(_subList, sub4);
+        assertNotNull("Should have been a node present for the subscription", sub4Node);
+
+        //verify the marked node (which is now a dummy substitute for the 3rd subscription) returns
+        //the 4th subscriptions node as the next non-deleted node.
+        assertEquals("Unexpected next node", sub4Node, _subList.getMarkedNode().findNext());
+    }
+
+    /**
      * Test that setting the marked node to null doesn't cause problems during remove operations
      */
     public void testRemoveWithNullMarkedNode()