Fix memory leak in Space#flyweights implementations (openrewrite#3368)

The `Space#flyweights` implementations all had a memory leak due to the way the `WeakHashMap` was being used: The values all had a hard reference to the key. As a result these entries can never get collected. Additionally, there was no length limitation on the whitespace, so very long spaces could end up in the flyweights table with a very low probability of ever getting reused.
rob-valor · Jun 27, 2023 · 068af07 · 068af07
1 parent 65d4c73
commit 068af07
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 6 deletions.
diff --git a/rewrite-hcl/src/main/java/org/openrewrite/hcl/tree/Space.java b/rewrite-hcl/src/main/java/org/openrewrite/hcl/tree/Space.java
@@ -61,8 +61,10 @@ public static Space build(@Nullable String whitespace, List<Comment> comments) {
         if (comments.isEmpty()) {
             if (whitespace == null || whitespace.isEmpty()) {
                 return Space.EMPTY;
+            } else if (whitespace.length() <= 100) {
+                //noinspection StringOperationCanBeSimplified
+                return flyweights.computeIfAbsent(new String(whitespace), k -> new Space(whitespace, comments));
             }
-            return flyweights.computeIfAbsent(whitespace, k -> new Space(whitespace, comments));
         }
         return new Space(whitespace, comments);
     }

diff --git a/rewrite-java/src/main/java/org/openrewrite/java/tree/Space.java b/rewrite-java/src/main/java/org/openrewrite/java/tree/Space.java
@@ -61,8 +61,10 @@ public static Space build(@Nullable String whitespace, List<Comment> comments) {
         if (comments.isEmpty()) {
             if (whitespace == null || whitespace.isEmpty()) {
                 return Space.EMPTY;
+            } else if (whitespace.length() <= 100) {
+                //noinspection StringOperationCanBeSimplified
+                return flyweights.computeIfAbsent(new String(whitespace), k -> new Space(whitespace, comments));
             }
-            return flyweights.computeIfAbsent(whitespace, k -> new Space(whitespace, comments));
         }
         return new Space(whitespace, comments);
     }

diff --git a/rewrite-json/src/main/java/org/openrewrite/json/tree/Space.java b/rewrite-json/src/main/java/org/openrewrite/json/tree/Space.java
@@ -60,8 +60,10 @@ public static Space build(@Nullable String whitespace, List<Comment> comments) {
         if (comments.isEmpty()) {
             if (whitespace == null || whitespace.isEmpty()) {
                 return Space.EMPTY;
+            } else if (whitespace.length() <= 100) {
+                //noinspection StringOperationCanBeSimplified
+                return flyweights.computeIfAbsent(new String(whitespace), k -> new Space(whitespace, comments));
             }
-            return flyweights.computeIfAbsent(whitespace, k -> new Space(whitespace, comments));
         }
         return new Space(whitespace, comments);
     }

diff --git a/rewrite-protobuf/src/main/java/org/openrewrite/protobuf/tree/Space.java b/rewrite-protobuf/src/main/java/org/openrewrite/protobuf/tree/Space.java
@@ -60,8 +60,10 @@ public static Space build(@Nullable String whitespace, List<Comment> comments) {
         if (comments.isEmpty()) {
             if (whitespace == null || whitespace.isEmpty()) {
                 return Space.EMPTY;
+            } else if (whitespace.length() <= 100) {
+                //noinspection StringOperationCanBeSimplified
+                return flyweights.computeIfAbsent(new String(whitespace), k -> new Space(whitespace, comments));
             }
-            return flyweights.computeIfAbsent(whitespace, k -> new Space(whitespace, comments));
         }
         return new Space(whitespace, comments);
     }

diff --git a/tools/language-parser-builder/src/main/java/org/openrewrite/toml/tree/Space.java b/tools/language-parser-builder/src/main/java/org/openrewrite/toml/tree/Space.java
@@ -48,8 +48,13 @@ private Space(@Nullable String whitespace) {
 
     @JsonCreator
     public static Space build(@Nullable String whitespace) {
-        if (whitespace == null || whitespace.isEmpty()) {
-            return Space.EMPTY;
+        if (comments.isEmpty()) {
+            if (whitespace == null || whitespace.isEmpty()) {
+                return Space.EMPTY;
+            } else if (whitespace.length() <= 100) {
+                //noinspection StringOperationCanBeSimplified
+                return flyweights.computeIfAbsent(new String(whitespace), k -> new Space(whitespace, comments));
+            }
         }
         return flyweights.computeIfAbsent(whitespace, k -> new Space(whitespace));
     }