You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by js...@apache.org on 2020/09/25 14:22:40 UTC

[sling-org-apache-sling-resourceresolver] 01/01: SLING-9769 - Minor memory leak in MapEntries class

This is an automated email from the ASF dual-hosted git repository.

jsedding pushed a commit to branch feature/SLING-9769-minor-memory-leak-in-MapEntries
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git

commit 1eac8dda6aa984e76b18cc1d42b0f2f1b0229d53
Author: Julian Sedding <js...@apache.org>
AuthorDate: Fri Sep 25 16:21:41 2020 +0200

    SLING-9769 - Minor memory leak in MapEntries class
---
 .../resourceresolver/impl/mapping/MapEntries.java  | 75 ++++++++--------------
 1 file changed, 26 insertions(+), 49 deletions(-)

diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
index ee6cf34..debc99d 100644
--- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
+++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
@@ -48,6 +48,8 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Supplier;
+import java.util.stream.Stream;
 
 import javax.servlet.http.HttpServletResponse;
 
@@ -140,7 +142,7 @@ public class MapEntries implements
 
     private Timer timer;
 
-    private boolean updateBloomFilterFile = false;
+    private final AtomicBoolean updateBloomFilterFile = new AtomicBoolean(false);
 
     private final StringInterpolationProvider stringInterpolationProvider;
 
@@ -235,6 +237,7 @@ public class MapEntries implements
                     log.debug("creating bloom filter file {}",
                             vanityBloomFilterFile.getAbsolutePath());
                     vanityBloomFilter = createVanityBloomFilter();
+                    updateBloomFilterFile.set(true);
                     persistBloomFilter();
                     createVanityBloomFilter = true;
                 } else {
@@ -247,8 +250,8 @@ public class MapEntries implements
                 }
 
                 // task for persisting the bloom filter every minute (if changes exist)
-                timer = new Timer();
-                timer.schedule(new BloomFilterTask(), 60_000);
+                timer = new Timer("VanityPathBloomFilterUpdater", true);
+                timer.schedule(new BloomFilterTask(), 60_000, 60_000);
 
                 this.vanityTargets = loadVanityPaths(createVanityBloomFilter);
             }
@@ -447,7 +450,7 @@ public class MapEntries implements
             needsUpdate = loadVanityPath(resource, resolveMapsMap, vanityTargets, false, true);
         }
         if ( needsUpdate ) {
-            updateBloomFilterFile = true;
+            updateBloomFilterFile.set(true);
             return true;
         }
         return false;
@@ -534,11 +537,10 @@ public class MapEntries implements
      * Cleans up this class.
      */
     public void dispose() {
-        try {
-            persistBloomFilter();
-        } catch (IOException e) {
-           log.error("Error while saving bloom filter to disk", e);
-        }
+
+        timer.cancel();
+
+        persistBloomFilter();
 
         if (this.registration != null) {
             this.registration.unregister();
@@ -620,7 +622,7 @@ public class MapEntries implements
     public Map<String, String> getAliasMap(final String parentPath) {
         return aliasMap.get(parentPath);
     }
-    
+
     @Override
     public Map<String, List<String>> getVanityPathMappings() {
         return Collections.unmodifiableMap(vanityTargets);
@@ -768,13 +770,14 @@ public class MapEntries implements
         return bloomFilter;
     }
 
-    private void persistBloomFilter() throws IOException {
-        if (vanityBloomFilterFile != null && vanityBloomFilter != null) {
-            FileOutputStream out = new FileOutputStream(vanityBloomFilterFile);
-            try {
+    private void persistBloomFilter() {
+        if (vanityBloomFilterFile != null && vanityBloomFilter != null
+                && updateBloomFilterFile.getAndSet(false)) {
+            try (FileOutputStream out = new FileOutputStream(vanityBloomFilterFile)) {
                 out.write(this.vanityBloomFilter);
-            } finally {
-                out.close();
+            } catch (IOException e) {
+                throw new RuntimeException(
+                        "Error while saving bloom filter to disk", e);
             }
         }
     }
@@ -1123,30 +1126,15 @@ public class MapEntries implements
         final String queryString = "SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base WHERE sling:vanityPath IS NOT NULL";
         final Iterator<Resource> i = resolver.findResources(queryString, "sql");
 
-        while (i.hasNext() && (createVanityBloomFilter || isAllVanityPathEntriesCached() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries())) {
+        Supplier<Boolean> isCacheComplete = () -> isAllVanityPathEntriesCached() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries();
+        while (i.hasNext() && (createVanityBloomFilter || isCacheComplete.get())) {
             final Resource resource = i.next();
-            boolean isValid = false;
-            for(final Path sPath : this.factory.getObservationPaths()) {
-                if ( sPath.matches(resource.getPath())) {
-                    isValid = true;
-                    break;
-                }
+            final String resourcePath = resource.getPath();
+            if (Stream.of(this.factory.getObservationPaths()).anyMatch(path -> path.matches(resourcePath))) {
+                loadVanityPath(resource, resolveMapsMap, targetPaths, isCacheComplete.get(), createVanityBloomFilter);
             }
-            if ( isValid ) {
-                if (isAllVanityPathEntriesCached() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries()) {
-                    // fill up the cache and the bloom filter
-                    loadVanityPath(resource, resolveMapsMap, targetPaths, true,
-                            createVanityBloomFilter);
-                } else {
-                    // fill up the bloom filter
-                    loadVanityPath(resource, resolveMapsMap, targetPaths, false,
-                            createVanityBloomFilter);
-                }
-            }
-
         }
 
-
         return targetPaths;
     }
 
@@ -1160,10 +1148,7 @@ public class MapEntries implements
         }
 
         final ValueMap props = resource.getValueMap();
-        long vanityOrder = 0;
-        if (props.containsKey(PROP_VANITY_ORDER)) {
-            vanityOrder = props.get(PROP_VANITY_ORDER, Long.class);
-        }
+        long vanityOrder = props.get(PROP_VANITY_ORDER, 0L);
 
         // url is ignoring scheme and host.port and the path is
         // what is stored in the sling:vanityPath property
@@ -1534,15 +1519,7 @@ public class MapEntries implements
     final class BloomFilterTask extends TimerTask {
         @Override
         public void run() {
-            try {
-                if (updateBloomFilterFile) {
-                    persistBloomFilter();
-                    updateBloomFilterFile = false;
-                }
-            } catch (IOException e) {
-                throw new RuntimeException(
-                        "Error while saving bloom filter to disk", e);
-            }
+            persistBloomFilter();
         }
     }