nikhiln64 commented on code in PR #6900:
URL: https://github.com/apache/shenyu/pull/6900#discussion_r3744086532
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/LeastActiveLoadBalance.java:
##########
@@ -45,6 +45,8 @@ protected Upstream doSelect(final List<Upstream>
upstreamList, final LoadBalance
.filter(key -> !countMap.containsKey(key))
.forEach(domain -> countMap.put(domain, Long.MIN_VALUE));
+ countMap.keySet().retainAll(domainMap.keySet());
Review Comment:
## Context
Thanks for tracking down the leak, the `countMap` really did grow without
bound before this. One thing I would want to sort out before it lands.
`LeastActiveLoadBalance` is a singleton (a bare @Join defaults to isSingleton
true, and ExtensionLoader caches one instance per algorithm), so `countMap` at
`LeastActiveLoadBalance.java:37` is a single JVM wide map shared by every
selector and rule that uses leastActive, keyed only by the upstream domain with
no selector namespacing. The retainAll you have added runs on every `doSelect`
against just the current request's upstream set, so two selectors with
different upstream lists end up deleting each other's entries.
> Example: If selector A serves {A1, A2} and selector B serves {B1, B2}, a
request through A removes B1 and B2 from the map, and the next request through
B removes A1 and A2 and re-adds B1 and B2 at `Long.MIN_VALUE`, discarding the
counts B had accumulated. Under interleaved traffic that reset happens on
nearly every request, so least active selection collapses toward near fixed
selection and the map churns rather than just leaking.
There is also a smaller lost update where a computeIfPresent increment no
ops if another thread's retainAll just removed that key.
## Suggestion
At least gating the cleanup on the selector's own size mismatch rather than
an unconditional `retainAll`, would fix the leak without touching other
selector.
### NOTE
Happy to be corrected if leastActive is only ever meant to serve one
selector at a time, but nothing in the SPI seems to enforce that and RoundRobin
assumes the opposite.
--
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]
