Issue checkstyle#2992: ThreadLocal usage in single-threaded checkstyle

Vladlis · Nov 27, 2016 · 44a5851 · 44a5851
1 parent f84e26a
commit 44a5851
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 33 deletions.
diff --git a/config/findbugs-exclude.xml b/config/findbugs-exclude.xml
@@ -112,4 +112,10 @@
         <Method name="makeCodeSelection"/>
         <Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
     </Match>
+    <Match>
+        <!-- Until https://github.com/checkstyle/checkstyle/issues/3573 -->
+        <Class name="com.puppycrawl.tools.checkstyle.checks.FileContentsHolder" />
+        <Method name="beginTree"/>
+        <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD"/>
+    </Match>
 </FindBugsFilter>
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/FileContentsHolder.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/FileContentsHolder.java
@@ -31,21 +31,20 @@
  * @author Mike McMahon
  * @author Rick Giles
  */
-public class FileContentsHolder
-    extends AbstractCheck {
+public class FileContentsHolder extends AbstractCheck {
     /** The current file contents. */
-    private static final ThreadLocal<FileContents> FILE_CONTENTS = new ThreadLocal<>();
+    private static FileContents currentFileContents;
 
     /**
      * @return the current file contents.
      */
-    public static FileContents getContents() {
-        return FILE_CONTENTS.get();
+    public static FileContents getCurrentFileContents() {
+        return currentFileContents;
     }
 
     @Override
     public int[] getDefaultTokens() {
-        return getAcceptableTokens();
+        return CommonUtils.EMPTY_INT_ARRAY;
     }
 
     @Override
@@ -60,14 +59,6 @@ public int[] getRequiredTokens() {
 
     @Override
     public void beginTree(DetailAST rootAST) {
-        FILE_CONTENTS.set(getFileContents());
-    }
-
-    @Override
-    public void destroy() {
-        // This needs to be called in destroy, rather than finishTree()
-        // as finishTree() is called before the messages are passed to the
-        // filters. Without calling remove, there is a memory leak.
-        FILE_CONTENTS.remove();
+        currentFileContents = getFileContents();
     }
 }
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithNearbyCommentFilter.java b/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithNearbyCommentFilter.java
@@ -191,16 +191,14 @@ public boolean accept(AuditEvent event) {
         if (event.getLocalizedMessage() != null) {
             // Lazy update. If the first event for the current file, update file
             // contents and tag suppressions
-            final FileContents currentContents = FileContentsHolder.getContents();
+            final FileContents currentContents = FileContentsHolder.getCurrentFileContents();
 
-            if (currentContents != null) {
-                if (getFileContents() != currentContents) {
-                    setFileContents(currentContents);
-                    tagSuppressions();
-                }
-                if (matchesTag(event)) {
-                    accepted = false;
-                }
+            if (getFileContents() != currentContents) {
+                setFileContents(currentContents);
+                tagSuppressions();
+            }
+            if (matchesTag(event)) {
+                accepted = false;
             }
         }
         return accepted;

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressionCommentFilter.java b/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressionCommentFilter.java
@@ -179,16 +179,14 @@ public boolean accept(AuditEvent event) {
         if (event.getLocalizedMessage() != null) {
             // Lazy update. If the first event for the current file, update file
             // contents and tag suppressions
-            final FileContents currentContents = FileContentsHolder.getContents();
+            final FileContents currentContents = FileContentsHolder.getCurrentFileContents();
 
-            if (currentContents != null) {
-                if (getFileContents() != currentContents) {
-                    setFileContents(currentContents);
-                    tagSuppressions();
-                }
-                final Tag matchTag = findNearestMatch(event);
-                accepted = matchTag == null || matchTag.isReportingOn();
+            if (getFileContents() != currentContents) {
+                setFileContents(currentContents);
+                tagSuppressions();
             }
+            final Tag matchTag = findNearestMatch(event);
+            accepted = matchTag == null || matchTag.isReportingOn();
         }
         return accepted;
     }

diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/FileSetCheckLifecycleTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/FileSetCheckLifecycleTest.java
@@ -121,7 +121,7 @@ protected void processFiltered(File file, List<String> lines) {
 
         @Override
         public void finishProcessing() {
-            fileContentAvailable = FileContentsHolder.getContents() != null;
+            fileContentAvailable = FileContentsHolder.getCurrentFileContents() != null;
         }
     }
 }