From 0bb6fa067fd3e2fbe1eb0846b5e299073d2df8a8 Mon Sep 17 00:00:00 2001 From: waw666waw666 <184799977+waw666waw666@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:12:38 +0000 Subject: [PATCH] Fix BitList positional operations after elements are removed BitList keeps the live elements of originList as set bits in rootSet, so once an element has been removed (e.g. an invoker filtered out by a router) a position in the list no longer equals an index in originList. Several java.util.List operations ignored that: - subList(from, to) cleared bits [0, from) and [to, ...) of rootSet, treating list positions as originList indexes, so it returned elements that are not in the list and dropped ones that are. - new BitList<>(bitList) set bits [0, bitList.size()) instead of copying rootSet, so the copy resurrected removed elements and lost others, and it shared the source's tailList, so changes to one leaked into the other. - BitListIterator.remove() did not reset its state, so a second remove() silently removed another element, and after removing the element returned by next() it kept the cursor on that element, so hasPrevious()/previous() still returned it. nextIndex() returned size - 1 at the end of the list, and listIterator(index) did not position correctly inside the tail list. Translate list positions through rootSet with a shared bitIndexOf helper, copy the source state in the copy constructor, and track the bit of the last returned element in the iterator. Add regression tests, including a randomized comparison of BitList's ListIterator against ArrayList's. --- .../rpc/cluster/router/state/BitList.java | 196 +++++++++--------- .../rpc/cluster/router/state/BitListTest.java | 146 +++++++++++++ 2 files changed, 247 insertions(+), 95 deletions(-) diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/state/BitList.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/state/BitList.java index 2ecc948b7c12..2e01d10ae039 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/state/BitList.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/state/BitList.java @@ -64,16 +64,20 @@ public BitList(List originList) { public BitList(List originList, boolean empty) { if (originList instanceof BitList) { - this.originList = ((BitList) originList).getOriginList(); - this.tailList = ((BitList) originList).getTailList(); + // Copy the source's view (origin list, live indexes and tail list) instead of + // assuming that the first size() elements of its origin list are still present. + BitList source = (BitList) originList; + synchronized (source) { + this.originList = source.originList; + this.rootSet = empty ? new BitSet() : (BitSet) source.rootSet.clone(); + this.tailList = empty || source.tailList == null ? null : new LinkedList<>(source.tailList); + } } else { this.originList = originList; - } - this.rootSet = new BitSet(); - if (!empty) { - this.rootSet.set(0, originList.size()); - } else { - this.tailList = null; + this.rootSet = new BitSet(); + if (!empty) { + this.rootSet.set(0, originList.size()); + } } } @@ -261,9 +265,21 @@ public synchronized void clear() { } } + /** + * Returns the index in {@code originList} of the element at the given position of this list, + * i.e. the position of the {@code (index + 1)}-th set bit of {@code rootSet}. + * {@code index} must be in {@code [0, rootSet.cardinality())}. + */ + private int bitIndexOf(int index) { + int bitIndex = -1; + for (int i = 0; i <= index; i++) { + bitIndex = rootSet.nextSetBit(bitIndex + 1); + } + return bitIndex; + } + @Override public synchronized E get(int index) { - int bitIndex = -1; if (index < 0) { throw new IndexOutOfBoundsException(); } @@ -274,16 +290,15 @@ public synchronized E get(int index) { throw new IndexOutOfBoundsException(); } } else { - for (int i = 0; i <= index; i++) { - bitIndex = rootSet.nextSetBit(bitIndex + 1); - } - return originList.get(bitIndex); + return originList.get(bitIndexOf(index)); } } @Override public synchronized E remove(int index) { - int bitIndex = -1; + if (index < 0) { + throw new IndexOutOfBoundsException(); + } if (index >= rootSet.cardinality()) { if (CollectionUtils.isNotEmpty(tailList)) { return tailList.remove(index - rootSet.cardinality()); @@ -291,9 +306,7 @@ public synchronized E remove(int index) { throw new IndexOutOfBoundsException(); } } else { - for (int i = 0; i <= index; i++) { - bitIndex = rootSet.nextSetBit(bitIndex + 1); - } + int bitIndex = bitIndexOf(index); rootSet.set(bitIndex, false); return originList.get(bitIndex); } @@ -370,26 +383,27 @@ public synchronized ListIterator listIterator(int index) { @Override public synchronized BitList subList(int fromIndex, int toIndex) { + int size = size(); + if (fromIndex < 0 || toIndex > size || fromIndex > toIndex) { + throw new IndexOutOfBoundsException( + "fromIndex: " + fromIndex + ", toIndex: " + toIndex + ", size: " + size); + } + // fromIndex and toIndex are positions in this list, not indexes of originList: + // translate them through rootSet, which may contain cleared bits. + int cardinality = rootSet.cardinality(); BitSet resultSet = (BitSet) rootSet.clone(); - List copiedTailList = tailList == null ? null : new LinkedList<>(tailList); - if (toIndex < size()) { - if (toIndex < rootSet.cardinality()) { - copiedTailList = null; - resultSet.set(toIndex, resultSet.length(), false); - } else { - copiedTailList = - copiedTailList == null ? null : copiedTailList.subList(0, toIndex - rootSet.cardinality()); - } + if (toIndex < cardinality) { + resultSet.clear(bitIndexOf(toIndex), resultSet.length()); } - if (fromIndex > 0) { - if (fromIndex < rootSet.cardinality()) { - resultSet.set(0, fromIndex, false); - } else { - resultSet.clear(); - copiedTailList = copiedTailList == null - ? null - : copiedTailList.subList(fromIndex - rootSet.cardinality(), copiedTailList.size()); - } + if (fromIndex < cardinality) { + resultSet.clear(0, bitIndexOf(fromIndex)); + } else { + resultSet.clear(); + } + List copiedTailList = null; + if (toIndex > cardinality) { + copiedTailList = + new LinkedList<>(tailList.subList(Math.max(fromIndex - cardinality, 0), toIndex - cardinality)); } return new BitList<>(originList, resultSet, copiedTailList); } @@ -401,25 +415,25 @@ public static class BitListIterator implements ListIterator { private int curBitIndex = -1; private boolean isInTailList = false; private int lastReturnedIndex = -1; + // index in originList of the element last returned, -1 if it came from tailList + private int lastReturnedBitIndex = -1; + // whether the element last returned came from next() (true) or previous() (false) + private boolean lastReturnedByNext; public BitListIterator(BitList bitList, int index) { this.bitList = bitList; + int cardinality = bitList.rootSet.cardinality(); + int tailSize = bitList.hasMoreElementInTailList() ? bitList.tailList.size() : 0; + if (index < 0 || index > cardinality + tailSize) { + throw new IndexOutOfBoundsException("Index: " + index + ", Size: " + (cardinality + tailSize)); + } this.index = index - 1; - for (int i = 0; i < index; i++) { - if (!isInTailList) { - curBitIndex = bitList.rootSet.nextSetBit(curBitIndex + 1); - if (curBitIndex == -1) { - if (CollectionUtils.isNotEmpty(bitList.tailList)) { - isInTailList = true; - tailListIterator = bitList.tailList.listIterator(); - tailListIterator.next(); - } else { - break; - } - } - } else { - tailListIterator.next(); - } + if (index <= cardinality) { + // the cursor is right after the index-th element of originList that is still present + curBitIndex = index == 0 ? -1 : bitList.bitIndexOf(index - 1); + } else { + isInTailList = true; + tailListIterator = bitList.tailList.listIterator(index - cardinality); } } @@ -439,31 +453,31 @@ public synchronized boolean hasNext() { @Override public synchronized E next() { + E next; if (isInTailList) { - if (tailListIterator.hasNext()) { - index += 1; - lastReturnedIndex = index; - } - return tailListIterator.next(); + next = tailListIterator.next(); + lastReturnedBitIndex = -1; } else { int nextBitIndex = bitList.rootSet.nextSetBit(curBitIndex + 1); if (nextBitIndex == -1) { if (bitList.hasMoreElementInTailList()) { tailListIterator = bitList.tailList.listIterator(); isInTailList = true; - index += 1; - lastReturnedIndex = index; - return tailListIterator.next(); + next = tailListIterator.next(); + lastReturnedBitIndex = -1; } else { throw new NoSuchElementException(); } } else { - index += 1; - lastReturnedIndex = index; curBitIndex = nextBitIndex; - return bitList.originList.get(nextBitIndex); + lastReturnedBitIndex = nextBitIndex; + next = bitList.originList.get(nextBitIndex); } } + index += 1; + lastReturnedIndex = index; + lastReturnedByNext = true; + return next; } @Override @@ -482,39 +496,30 @@ public synchronized boolean hasPrevious() { @Override public synchronized E previous() { - if (isInTailList) { - boolean hasPreviousInTailList = tailListIterator.hasPrevious(); - if (hasPreviousInTailList) { - lastReturnedIndex = index; - index -= 1; - return tailListIterator.previous(); - } else { - int lastIndexInBit = bitList.rootSet.previousSetBit(bitList.rootSet.size()); - if (lastIndexInBit == -1) { - throw new NoSuchElementException(); - } else { - isInTailList = false; - curBitIndex = bitList.rootSet.previousSetBit(lastIndexInBit - 1); - lastReturnedIndex = index; - index -= 1; - return bitList.originList.get(lastIndexInBit); - } - } + E previous; + if (isInTailList && tailListIterator.hasPrevious()) { + previous = tailListIterator.previous(); + lastReturnedBitIndex = -1; } else { - if (curBitIndex == -1) { + int previousBitIndex = + isInTailList ? bitList.rootSet.previousSetBit(bitList.rootSet.size()) : curBitIndex; + if (previousBitIndex == -1) { throw new NoSuchElementException(); } - int nextBitIndex = curBitIndex; - curBitIndex = bitList.rootSet.previousSetBit(curBitIndex - 1); - lastReturnedIndex = index; - index -= 1; - return bitList.originList.get(nextBitIndex); + isInTailList = false; + curBitIndex = bitList.rootSet.previousSetBit(previousBitIndex - 1); + lastReturnedBitIndex = previousBitIndex; + previous = bitList.originList.get(previousBitIndex); } + lastReturnedIndex = index; + index -= 1; + lastReturnedByNext = false; + return previous; } @Override public synchronized int nextIndex() { - return hasNext() ? index + 1 : index; + return index + 1; } @Override @@ -526,20 +531,21 @@ public synchronized int previousIndex() { public synchronized void remove() { if (lastReturnedIndex == -1) { throw new IllegalStateException(); + } + if (lastReturnedBitIndex == -1) { + tailListIterator.remove(); } else { - if (lastReturnedIndex >= bitList.rootSet.cardinality()) { - tailListIterator.remove(); - } else { - int bitIndex = -1; - for (int i = 0; i <= lastReturnedIndex; i++) { - bitIndex = bitList.rootSet.nextSetBit(bitIndex + 1); - } - bitList.rootSet.set(bitIndex, false); + bitList.rootSet.clear(lastReturnedBitIndex); + if (lastReturnedBitIndex == curBitIndex) { + // removed the element right before the cursor, step the cursor back over it + curBitIndex = bitList.rootSet.previousSetBit(curBitIndex - 1); } } - if (lastReturnedIndex <= index) { + if (lastReturnedByNext) { index -= 1; } + lastReturnedIndex = -1; + lastReturnedBitIndex = -1; } @Override diff --git a/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/router/state/BitListTest.java b/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/router/state/BitListTest.java index 6b49fd9d75ea..5ce32e814be5 100644 --- a/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/router/state/BitListTest.java +++ b/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/router/state/BitListTest.java @@ -16,12 +16,14 @@ */ package org.apache.dubbo.rpc.cluster.router.state; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.ListIterator; +import java.util.Random; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicReference; @@ -637,4 +639,148 @@ void testConcurrent() throws InterruptedException { Assertions.assertDoesNotThrow(() -> ref.get().iterator().hasNext()); } } + + @Test + void testSubListAfterRemove() { + // B and D have been removed (e.g. filtered out by a router), so the list is [A, C, E] + [F, G] + BitList bitList = new BitList<>(Arrays.asList("A", "B", "C", "D", "E")); + bitList.remove("B"); + bitList.remove("D"); + bitList.add("F"); + bitList.add("G"); + List expected = Arrays.asList("A", "C", "E", "F", "G"); + Assertions.assertEquals(expected, bitList); + + for (int from = 0; from <= expected.size(); from++) { + for (int to = from; to <= expected.size(); to++) { + BitList subList = bitList.subList(from, to); + Assertions.assertEquals(expected.subList(from, to), subList, "subList(" + from + ", " + to + ")"); + Assertions.assertEquals(to - from, subList.size()); + } + } + + Assertions.assertThrows(IndexOutOfBoundsException.class, () -> bitList.subList(-1, 2)); + Assertions.assertThrows(IndexOutOfBoundsException.class, () -> bitList.subList(0, 6)); + Assertions.assertThrows(IndexOutOfBoundsException.class, () -> bitList.subList(3, 2)); + } + + @Test + void testCopyConstructorFromBitList() { + BitList bitList = new BitList<>(Arrays.asList("A", "B", "C", "D", "E")); + bitList.remove("A"); + bitList.remove("C"); + bitList.add("F"); + + BitList copy = new BitList<>(bitList); + Assertions.assertEquals(Arrays.asList("B", "D", "E", "F"), copy); + Assertions.assertEquals(bitList, copy); + + // the copy is independent of the source + copy.add("G"); + copy.remove("F"); + copy.remove("B"); + Assertions.assertEquals(Arrays.asList("B", "D", "E", "F"), bitList); + Assertions.assertEquals(Arrays.asList("D", "E", "G"), copy); + + BitList emptyCopy = new BitList<>(bitList, true); + Assertions.assertTrue(emptyCopy.isEmpty()); + Assertions.assertSame(bitList.getOriginList(), emptyCopy.getOriginList()); + } + + @Test + void testListIteratorRemove() { + BitList bitList = new BitList<>(Arrays.asList("A", "B", "C", "D")); + bitList.remove("B"); + bitList.add("E"); + List expected = new LinkedList<>(Arrays.asList("A", "C", "D", "E")); + Assertions.assertEquals(expected, bitList); + + ListIterator iterator = bitList.listIterator(); + ListIterator expectedIterator = expected.listIterator(); + + Assertions.assertEquals(expectedIterator.next(), iterator.next()); + iterator.remove(); + expectedIterator.remove(); + // a second remove() without next()/previous() must not remove another element + Assertions.assertThrows(IllegalStateException.class, iterator::remove); + Assertions.assertEquals(expected, bitList); + // the removed element must not be returned by previous() + Assertions.assertEquals(expectedIterator.hasPrevious(), iterator.hasPrevious()); + Assertions.assertEquals(expectedIterator.nextIndex(), iterator.nextIndex()); + Assertions.assertEquals(expectedIterator.previousIndex(), iterator.previousIndex()); + + Assertions.assertEquals(expectedIterator.next(), iterator.next()); + Assertions.assertEquals(expectedIterator.next(), iterator.next()); + Assertions.assertEquals(expectedIterator.previous(), iterator.previous()); + iterator.remove(); + expectedIterator.remove(); + Assertions.assertEquals(expected, bitList); + Assertions.assertEquals(expectedIterator.nextIndex(), iterator.nextIndex()); + Assertions.assertEquals(expectedIterator.previous(), iterator.previous()); + + while (expectedIterator.hasNext()) { + Assertions.assertTrue(iterator.hasNext()); + Assertions.assertEquals(expectedIterator.nextIndex(), iterator.nextIndex()); + Assertions.assertEquals(expectedIterator.next(), iterator.next()); + } + Assertions.assertFalse(iterator.hasNext()); + // at the end of the list nextIndex() is the list size + Assertions.assertEquals(expected.size(), iterator.nextIndex()); + Assertions.assertEquals(expected.size() - 1, iterator.previousIndex()); + } + + @Test + void testListIteratorMatchesArrayList() { + Random random = new Random(0); + for (int round = 0; round < 2000; round++) { + List origin = new ArrayList<>(); + int originSize = random.nextInt(6); + for (int i = 0; i < originSize; i++) { + origin.add("o" + i); + } + BitList bitList = new BitList<>(origin); + for (int i = 0; i < originSize; i++) { + if (random.nextBoolean()) { + bitList.remove("o" + i); + } + } + int tailSize = random.nextInt(3); + for (int i = 0; i < tailSize; i++) { + bitList.add("t" + i); + } + List expected = new ArrayList<>(bitList); + + int start = random.nextInt(expected.size() + 1); + ListIterator iterator = bitList.listIterator(start); + ListIterator expectedIterator = expected.listIterator(start); + for (int step = 0; step < 10; step++) { + String context = "round " + round + " step " + step; + switch (random.nextInt(3)) { + case 0: + Assertions.assertEquals(expectedIterator.hasNext(), iterator.hasNext(), context); + if (expectedIterator.hasNext()) { + Assertions.assertEquals(expectedIterator.next(), iterator.next(), context); + } + break; + case 1: + Assertions.assertEquals(expectedIterator.hasPrevious(), iterator.hasPrevious(), context); + if (expectedIterator.hasPrevious()) { + Assertions.assertEquals(expectedIterator.previous(), iterator.previous(), context); + } + break; + default: + try { + expectedIterator.remove(); + } catch (IllegalStateException e) { + Assertions.assertThrows(IllegalStateException.class, iterator::remove, context); + break; + } + iterator.remove(); + } + Assertions.assertEquals(expectedIterator.nextIndex(), iterator.nextIndex(), context); + Assertions.assertEquals(expectedIterator.previousIndex(), iterator.previousIndex(), context); + Assertions.assertEquals(expected, bitList, context); + } + } + } }