PDGGK opened a new pull request, #39323:
URL: https://github.com/apache/shardingsphere/pull/39323
## Changes
`RoundRobinLoadBalanceAlgorithm.getTargetName` indexes with
`Math.abs(count.getAndIncrement()) % availableTargetNames.size()`.
`Math.abs(Integer.MIN_VALUE)` is `Integer.MIN_VALUE` — `abs` cannot represent
`2^31` in an `int` — so once the counter wraps, the remainder is negative and
`List.get` throws.
```
java.lang.ArrayIndexOutOfBoundsException: Index -2 out of bounds for length 3
at
RoundRobinLoadBalanceAlgorithm.getTargetName(RoundRobinLoadBalanceAlgorithm.java:36)
```
Replaced with `Math.floorMod(count.getAndIncrement(),
availableTargetNames.size())`.
Only replica groups whose size is not a power of two are affected — for 2, 4
or 8 the negative remainder happens to be 0:
| group size | `Math.abs(Integer.MIN_VALUE) % size` | |
| --- | --- | --- |
| 2 | 0 | survives |
| 3 | **-2** | throws |
| 4 | 0 | survives |
| 5 | **-3** | throws |
| 8 | 0 | survives |
The existing test uses a two-element list, which is why this was never
caught.
## Why `floorMod` is safe below the wrap
`Math.floorMod(i, size)` and `Math.abs(i) % size` are identical for every
non-negative `i`, so nothing changes until the counter overflows. I checked
that exhaustively rather than assuming it — all counters in `[0, 5_000_000]`
against group sizes 1..16, 80,000,016 pairs, zero differences. The only
behavioural change is at the wrap itself, where one target is selected twice in
a row instead of an exception being thrown.
## Prior art
#1265 reported this same `ArrayIndexOutOfBoundsException` in 2018 and was
closed as completed; the fix then was a
`count.compareAndSet(readDataSourceNames.size(), 0)` line that kept the counter
bounded. #17422 removed that line in 2022 while changing the counter from a
static map to an instance field, and kept the `Math.abs(...)` expression.
To be accurate about it: that old line was itself racy — several threads
could step past `size` before any of them observed it — so the point is not
that a correct guard was lost, but that the counter has had no bound at all
since then, and `getTargetName` is annotated `@HighFrequencyInvocation`.
## Testing
Added `assertGetAvailableTargetNameWhenCounterOverflows`, which seeds
`count` with `Integer.MAX_VALUE` using the repo's `Plugins.getMemberAccessor()`
idiom and calls the algorithm three times against a three-element list. It
reproduces the exception on `master` and passes with this change.
- `RoundRobinLoadBalanceAlgorithmTest` — fails before, 2/2 after
- `infra/algorithm/type/load-balancer/type/round-robin` +
`features/readwrite-splitting/core` with `-am` — 3404 tests, 0 failures, 0
errors
- `checkstyle:check` with `src/resources/checkstyle.xml` — clean
Fixes #39319
## Type
- [x] Bugfix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
