ctubbsii commented on a change in pull request #1965:
URL: https://github.com/apache/accumulo/pull/1965#discussion_r593772091
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/ratelimit/SharedRateLimiterFactory.java
##########
@@ -112,9 +112,9 @@ protected void update() {
}
for (Map.Entry<String,WeakReference<SharedRateLimiter>> entry :
limitersCopy.entrySet()) {
try {
- if (Objects.nonNull(entry.getValue().get())) {
- entry.getValue().get().update();
- }
+ Objects.requireNonNull(entry.getValue().get()).update();
+ } catch (NullPointerException npex) {
Review comment:
Spotbugs is signaling that WeakReferences stored in the map may actually
be a reference to null, if the referenced object had been garbage collected,
but not yet removed from the map. It would be better to avoid inserting any
weak references into this copy, since the whole point is to get strong
references to call update on them. It'd probably be enough to simply have an
`if` statement to check to see if they are null before calling `update` on them
after the copy.
Alternatively, you could map the values by calling `WeakReference.get()` on
the values, and filtering out the null ones. And, since we don't use the keys,
it's even easier. Something like:
```java
List<SharedRateLimiter> limitersCopy = new ArrayList<>();
synchronized(activeLimiters) {
limitersCopy =
activeLImiters.values().stream().filter(Objects::nonNull).collect(Collectors.toList());
}
limitersCopy.forEach(SharedRateLimiter::update);
```
Of course, it might not be exactly that simple, because I just realized we
*do* use the keys for the log message if there's an exception. But hopefully
you get the point... filter out or check for nulls from the copy before calling
update, because WeakReference.get() can be null.
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/ratelimit/SharedRateLimiterFactory.java
##########
@@ -87,12 +90,12 @@ public static synchronized SharedRateLimiterFactory
getInstance(AccumuloConfigur
public RateLimiter create(String name, RateProvider rateProvider) {
synchronized (activeLimiters) {
if (activeLimiters.containsKey(name)) {
- return activeLimiters.get(name);
+ return activeLimiters.get(name).get();
} else {
long initialRate;
initialRate = rateProvider.getDesiredRate();
SharedRateLimiter limiter = new SharedRateLimiter(name, rateProvider,
initialRate);
- activeLimiters.put(name, limiter);
+ activeLimiters.put(name, new WeakReference<>(limiter));
return limiter;
Review comment:
The typical pattern with WeakReferences in a WeakHashMap should be to
check if the `weakReference.get()` is null. If it's null, it shouldn't be
returned, but instead recomputed, as in the `else` clause. That is because map
entries in the WeakHashMap may not be cleaned up yet, even if the WeakReference
is already null.
Also, this whole if/else block may be more easily implemented inside a
lambda passed to `activeLimiters.compute()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
