Repository navigation
Fix BitList positional operations after elements are removed - #16578
Open
waw666waw666 wants to merge 1 commit into
Open
waw666waw666 wants to merge 1 commit into
waw666waw666 wants to merge 1 commit into
Conversation
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16578 +/- ##
============================================
+ Coverage 61.00% 61.02% +0.01%
- Complexity 15 11760 +11745
============================================
Files 1953 1953
Lines 89286 89285 -1
Branches 13478 13472 -6
============================================
+ Hits 54471 54485 +14
+ Misses 29213 29201 -12
+ Partials 5602 5599 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change?
BitListstores the live elements oforiginListas set bits inrootSet. Once an element has been removed, for example an invoker filtered out by a state router (the normal state of a routed invoker list), a position in the list is no longer the same as an index inoriginList.get(int),remove(int)andindexOfhandle this already. Thesejava.util.Listoperations don't:subList(from, to)clears bits[0, from)and[to, ...)ofrootSet. It treats list positions asoriginListindexes, so it returns elements that are not in the list and drops elements that are.new BitList<>(bitList)sets bits[0, bitList.size())instead of copyingrootSet. The copy brings back removed elements and loses live ones. It also shares the source'stailList, so changes to the copy leak into the source. With a tail list, iterating the copy can throwIndexOutOfBoundsException.BitListIterator:remove()does not reset its state, so a secondremove()doesn't throwIllegalStateException. It silently removes another element instead.next(), the cursor still points at that element, sohasPrevious()andprevious()still return it.nextIndex()returnssize - 1at the end of the list.listIterator(index)does not check its bounds and doesn't position correctly inside the tail list.Reproduction on current
3.3(2d9730c):Code that treats a routed
BitListas a plainListcan therefore get invokers that were routed away, or lose valid ones. That includesRouterSnapshotNode, which printssubList(0, 5)of each node's invokers, and custom routers or load balancers.What this PR changes:
bitIndexOf(int)helper that maps a list position to itsoriginListindex.get(int)andremove(int)now use it instead of repeating the loop, andremove(int)rejects negative indexes likeget(int)does.subListtranslatesfrom/tothroughrootSet, copies only the needed part of the tail list, and checks its bounds the wayList.subListrequires.rootSetandtailListwhen the source is aBitList.BitListIteratorkeeps theoriginListindex of the last returned element.remove()clears exactly that bit (no more O(n) scan per removal), steps the cursor back when the removed element was returned bynext(), and resets its state.nextIndex()is alwayspreviousIndex() + 1. The constructor positions the cursor directly, including inside the tail list.removeIfand plain forward iteration, which the routers use, behave as before. They just no longer scan the bit set on everyremove().Tests:
BitListTestgets four new tests:subListover every range of a list with removed elements and a tail, the copy constructor and its independence from the source,ListIterator.remove()semantics, and a seeded randomized comparison ofBitList.listIterator(i)againstArrayList(next/previous/remove/nextIndex/previousIndex, with and without tail elements). All four fail on3.3and pass with this change.mvn -pl dubbo-cluster testpasses (314 tests), and so dospotless:checkandcheckstyle:check.Checklist