Fix the race condition introduced with 3470888

### What's done: - Original issue: #1548. - Original PR: #1553. - This change reverts 3470888. - Enhances the KDoc of `DiktatRule`. - Fixes #1548.
saveourtool · Nov 9, 2022 · 70099a7 · 70099a7
1 parent 0cb47bb
commit 70099a7
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 28 deletions.
diff --git a/diktat-common/src/main/kotlin/org/cqfn/diktat/common/config/rules/RulesConfigReader.kt b/diktat-common/src/main/kotlin/org/cqfn/diktat/common/config/rules/RulesConfigReader.kt
@@ -6,6 +6,7 @@ package org.cqfn.diktat.common.config.rules
 
 import org.cqfn.diktat.common.config.reader.JsonResourceConfigReader
 import org.cqfn.diktat.common.config.rules.RulesConfigReader.Companion.log
+import org.cqfn.diktat.common.utils.LazyMessage
 import org.cqfn.diktat.common.utils.loggerWithKtlintConfig
 
 import com.charleskorn.kaml.Yaml
@@ -16,6 +17,7 @@ import mu.KotlinLogging
 import java.io.BufferedReader
 import java.io.File
 import java.util.Locale
+import java.util.concurrent.atomic.AtomicBoolean
 import java.util.concurrent.atomic.AtomicInteger
 
 import kotlinx.serialization.ExperimentalSerializationApi
@@ -99,19 +101,30 @@ open class RulesConfigReader(override val classLoader: ClassLoader) : JsonResour
     override fun getConfigFile(resourceFileName: String): BufferedReader? {
         val resourceFile = File(resourceFileName)
         return if (resourceFile.exists()) {
-            log.debug("Using diktat-analysis.yml file from the following path: ${resourceFile.absolutePath}")
+            log.debugOnce {
+                "Using $DIKTAT_ANALYSIS_CONF file from the following path: ${resourceFile.absolutePath}"
+            }
             File(resourceFileName).bufferedReader()
         } else {
-            log.debug("Using the default diktat-analysis.yml file from the class path")
+            log.debugOnce {
+                "Using the default $DIKTAT_ANALYSIS_CONF file from the class path"
+            }
             classLoader.getResourceAsStream(resourceFileName)?.bufferedReader()
         }
     }
 
     companion object {
+        internal val log: KLogger = KotlinLogging.loggerWithKtlintConfig(RulesConfigReader::class)
+        private val debugMessageLogged = AtomicBoolean()
+
         /**
-         * A [Logger] that can be used
+         * Don't use with more than a single [lazyMessage].
          */
-        val log: KLogger = KotlinLogging.loggerWithKtlintConfig(RulesConfigReader::class)
+        private fun KLogger.debugOnce(lazyMessage: LazyMessage) {
+            if (debugMessageLogged.compareAndSet(false, true)) {
+                debug(lazyMessage)
+            }
+        }
     }
 }
 

diff --git a/diktat-common/src/main/kotlin/org/cqfn/diktat/common/utils/LoggingUtils.kt b/diktat-common/src/main/kotlin/org/cqfn/diktat/common/utils/LoggingUtils.kt
@@ -8,6 +8,11 @@ import com.pinterest.ktlint.core.initKtLintKLogger
 import mu.KotlinLogging
 import kotlin.reflect.KClass
 
+/**
+ * Lazy string message.
+ */
+typealias LazyMessage = () -> String
+
 /**
  * Create a logger using [KotlinLogging] and configure it by ktlint's mechanism
  *

diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRule.kt
@@ -14,7 +14,7 @@ import org.jetbrains.kotlin.com.intellij.lang.ASTNode
 private typealias DiktatConfigRule = org.cqfn.diktat.common.config.rules.Rule
 
 /**
- * This is a wrapper around Ktlint Rule
+ * This is a wrapper around _KtLint_ `Rule`.
  *
  * @param id id of the rule
  * @property configRules all rules from configuration
@@ -33,7 +33,17 @@ abstract class DiktatRule(
     var isFixMode: Boolean = false
 
     /**
-     * Will be initialized in visit
+     * The **file-specific** error emitter, initialized in
+     * [beforeVisitChildNodes] and used in [logic] implementations.
+     *
+     * Since the file is indirectly a part of the state of a `Rule`, the same
+     * `Rule` instance should **never be re-used** to check more than a single
+     * file, or confusing effects (incl. race conditions) will occur.
+     * See the documentation of the [Rule] class for more details.
+     *
+     * @see Rule
+     * @see beforeVisitChildNodes
+     * @see logic
      */
     lateinit var emitWarn: EmitType
 

diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt
@@ -10,6 +10,7 @@ import org.cqfn.diktat.common.config.rules.DIKTAT_CONF_PROPERTY
 import org.cqfn.diktat.common.config.rules.DIKTAT_RULE_SET_ID
 import org.cqfn.diktat.common.config.rules.RulesConfig
 import org.cqfn.diktat.common.config.rules.RulesConfigReader
+import org.cqfn.diktat.common.utils.LazyMessage
 import org.cqfn.diktat.common.utils.loggerWithKtlintConfig
 import org.cqfn.diktat.ruleset.constants.Warnings
 import org.cqfn.diktat.ruleset.rules.OrderedRuleSet.Companion.ordered
@@ -90,10 +91,12 @@ import org.cqfn.diktat.ruleset.rules.chapter6.classes.StatelessClassesRule
 
 import com.pinterest.ktlint.core.RuleSet
 import com.pinterest.ktlint.core.RuleSetProvider
+import mu.KLogger
 import mu.KotlinLogging
 import org.jetbrains.kotlin.org.jline.utils.Levenshtein
 
 import java.io.File
+import java.util.concurrent.atomic.AtomicBoolean
 
 /**
  * [RuleSetProvider] that provides diKTat ruleset.
@@ -109,31 +112,32 @@ class DiktatRuleSetProvider(private var diktatConfigFile: String = DIKTAT_ANALYS
         yield(resolveConfigFileFromSystemProperty())
     }
 
-    /**
-     * As of _KtLint_ **0.47**, each rule is expected to have a state and is executed
-     * twice per file, and a new `Rule` instance is created per each file checked.
-     *
-     * Diktat rules have no mutable state yet and use the deprecated _KtLint_
-     * API, so we initialize them only _once_ for performance reasons and also
-     * to avoid redundant logging.
-     */
-    private val ruleSet: RuleSet by lazy {
-        log.debug("Will run $DIKTAT_RULE_SET_ID with $diktatConfigFile" +
-                " (it can be placed to the run directory or the default file from resources will be used)")
+    @Suppress(
+        "LongMethod",
+        "TOO_LONG_FUNCTION",
+    )
+    @Deprecated(
+        "Marked for removal in KtLint 0.48. See changelog or KDoc for more information.",
+    )
+    override fun get(): RuleSet {
+        log.debugOnce {
+            "Will run $DIKTAT_RULE_SET_ID with $diktatConfigFile" +
+                    " (it can be placed to the run directory or the default file from resources will be used)"
+        }
         val configPath = possibleConfigs
             .firstOrNull { it != null && File(it).exists() }
         diktatConfigFile = configPath
             ?: run {
-                val possibleConfigsList = possibleConfigs.toList()
-                log.warn(
+                log.warnOnce {
+                    val possibleConfigsList = possibleConfigs.toList()
                     "Configuration file not found in directory where diktat is run (${possibleConfigsList[0]}) " +
                             "or in the directory where diktat.jar is stored (${possibleConfigsList[1]}) " +
                             "or in system property <diktat.config.path> (${possibleConfigsList[2]}), " +
                             "the default file included in jar will be used. " +
                             "Some configuration options will be disabled or substituted with defaults. " +
                             "Custom configuration file should be placed in diktat working directory if run from CLI " +
                             "or provided as configuration options in plugins."
-                )
+                }
                 diktatConfigFile
             }
 
@@ -229,18 +233,12 @@ class DiktatRuleSetProvider(private var diktatConfigFile: String = DIKTAT_ANALYS
             .map {
                 it.invoke(configRules)
             }
-        RuleSet(
+        return RuleSet(
             DIKTAT_RULE_SET_ID,
             rules = rules.toTypedArray()
         ).ordered()
     }
 
-    @Deprecated(
-        "Marked for removal in KtLint 0.48. See changelog or KDoc for more information.",
-    )
-    override fun get(): RuleSet =
-        ruleSet
-
     private fun validate(config: RulesConfig) =
         require(config.name == DIKTAT_COMMON || config.name in Warnings.names) {
             val closestMatch = Warnings.names.minByOrNull { Levenshtein.distance(it, config.name) }
@@ -272,5 +270,37 @@ class DiktatRuleSetProvider(private var diktatConfigFile: String = DIKTAT_ANALYS
 
     companion object {
         private val log = KotlinLogging.loggerWithKtlintConfig(DiktatRuleSetProvider::class)
+
+        /**
+         * @see warnMessageLogged
+         */
+        private val debugMessageLogged = AtomicBoolean()
+
+        /**
+         * @see debugMessageLogged
+         */
+        private val warnMessageLogged = AtomicBoolean()
+
+        /**
+         * Don't use with more than a single [lazyMessage].
+         *
+         * @see KLogger.warnOnce
+         */
+        private fun KLogger.debugOnce(lazyMessage: LazyMessage) {
+            if (debugMessageLogged.compareAndSet(false, true)) {
+                debug(lazyMessage)
+            }
+        }
+
+        /**
+         * Don't use with more than a single [lazyMessage].
+         *
+         * @see KLogger.debugOnce
+         */
+        private fun KLogger.warnOnce(lazyMessage: LazyMessage) {
+            if (warnMessageLogged.compareAndSet(false, true)) {
+                warn(lazyMessage)
+            }
+        }
     }
 }
diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/util/TestUtils.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/util/TestUtils.kt
@@ -8,6 +8,7 @@
 
 package org.cqfn.diktat.util
 
+import org.cqfn.diktat.common.utils.LazyMessage
 import org.cqfn.diktat.ruleset.constants.EmitType
 
 import com.pinterest.ktlint.core.KtLint
@@ -28,7 +29,7 @@ internal const val TEST_FILE_NAME = "TestFileName.kt"
  * @param lazyFailureMessage the message to evaluate in case of a failure.
  * @return a non-`null` value.
  */
-internal fun <T> T?.assertNotNull(lazyFailureMessage: () -> String = { "Expecting actual not to be null" }): T =
+internal fun <T> T?.assertNotNull(lazyFailureMessage: LazyMessage = { "Expecting actual not to be null" }): T =
     this ?: fail(lazyFailureMessage())
 
 /**