From 2f1462b621da0d19db8a394b6337b2dccc6891fe Mon Sep 17 00:00:00 2001
From: Andreas Dangel <andreas.dangel@pmd-code.org>
Date: Fri, 22 Jul 2022 12:23:13 +0200
Subject: [PATCH] [core] Fix lazy init for allSuppressors in
 DefaultRuleViolationFactory

Fixes #4035
---
 docs/pages/7_0_0_release_notes.md             |  2 ++
 .../impl/DefaultRuleViolationFactory.java     | 22 +++++++++++++------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/docs/pages/7_0_0_release_notes.md b/docs/pages/7_0_0_release_notes.md
index be94436737..de1b50f21d 100644
--- a/docs/pages/7_0_0_release_notes.md
+++ b/docs/pages/7_0_0_release_notes.md
@@ -184,6 +184,8 @@ The following previously deprecated rules have been finally removed:
 * miscellaneous
     *   [#896](https://github.com/pmd/pmd/issues/896): \[all] Use slf4j
     *   [#1451](https://github.com/pmd/pmd/issues/1451): \[core] RulesetFactoryCompatibility stores the whole ruleset file in memory as a string
+* core
+    * [#4035](https://github.com/pmd/pmd/issues/4035): \[core] ConcurrentModificationException in DefaultRuleViolationFactory
 * cli
     *   [#3828](https://github.com/pmd/pmd/issues/3828): \[core] Progress reporting
 * apex-design
diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/impl/DefaultRuleViolationFactory.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/impl/DefaultRuleViolationFactory.java
index e44088831b..be1560ed2d 100644
--- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/impl/DefaultRuleViolationFactory.java
+++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/impl/DefaultRuleViolationFactory.java
@@ -26,7 +26,9 @@ public class DefaultRuleViolationFactory implements RuleViolationFactory {
     // todo move to package reporting
 
     private static final DefaultRuleViolationFactory DEFAULT = new DefaultRuleViolationFactory();
-    private Set<ViolationSuppressor> allSuppressors;
+
+    // volatile for lazy init - see #getAllSuppressors
+    private volatile Set<ViolationSuppressor> allSuppressors; // NOPMD volatile needed for lazy init
 
     @Override
     public SuppressedViolation suppressOrNull(Node location, RuleViolation violation) {
@@ -48,16 +50,22 @@ public class DefaultRuleViolationFactory implements RuleViolationFactory {
     }
 
     private Set<ViolationSuppressor> getAllSuppressors() {
-        if (allSuppressors == null) {
+        Set<ViolationSuppressor> result = allSuppressors;
+        if (result == null) {
             // lazy loaded because calling getSuppressors in constructor
             // is not safe wrt initialization of static constants
             // (order dependent otherwise)
-            this.allSuppressors = new LinkedHashSet<>(getSuppressors());
-            allSuppressors.add(ViolationSuppressor.NOPMD_COMMENT_SUPPRESSOR);
-            allSuppressors.add(ViolationSuppressor.REGEX_SUPPRESSOR);
-            allSuppressors.add(ViolationSuppressor.XPATH_SUPPRESSOR);
+            result = new LinkedHashSet<>(getSuppressors());
+            result.add(ViolationSuppressor.NOPMD_COMMENT_SUPPRESSOR);
+            result.add(ViolationSuppressor.REGEX_SUPPRESSOR);
+            result.add(ViolationSuppressor.XPATH_SUPPRESSOR);
+
+            // note 1: allSuppressors must be volatile to avoid other threads seeing the HashSet under construction
+            // note 2: multiple threads might create their own HashSets and the last HashSet is stored and overwrites
+            // previously created HashSets. This is ok, because this method is supposed to be idempotent.
+            allSuppressors = result;
         }
-        return allSuppressors;
+        return result;
     }
 
     /** Returns the default instance (no additional suppressors, creates a ParametricRuleViolation). */