[GitHub] [accumulo] ctubbsii commented on a change in pull request #1965: Fix WeakHashMap implementation in SharedRateLimiterFactory

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org