From 718f6d9e0ae07d698f86d8741761176c8ffb7705 Mon Sep 17 00:00:00 2001
From: Andrei Klunnyi <andrei.klunnyi@jetbrains.com>
Date: Tue, 5 Dec 2023 14:31:39 +0100
Subject: [PATCH] [kotlin][scripting] KTIJ-27951 IDEA hangs right after start

There are three factors to take into account:

1. `ScriptTemplatesFromDependenciesProvider.loadSync` launches synchronous retryable
    read-action. Read, it can block current thread for a significant amount of time.
2. The method is called under `ScriptDefinitionsManager.NewLogicDelegate.definitionsLock`
   potentially holding it for a long time.
3. The outer `ScriptDefinitionsManager.NewLogicDelegate.reloadDefinitionsInternal`
   can be called under global read-lock.

All these factors lead to the situation when the global write-action cannot be acquired.
Therefore, UI freezes.

This commit extracts [1] from [2] thus preventing the situation when the global read lock
is acquired under a local one.

GitOrigin-RevId: 53f3284981391bac7a1f539330dc96884177e235
---
 .../core/script/ScriptDefinitionsManager.kt   | 35 ++++++++++---------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/plugins/kotlin/base/scripting/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt b/plugins/kotlin/base/scripting/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt
index ff63b1b56348..19b78722b730 100644
--- a/plugins/kotlin/base/scripting/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt
+++ b/plugins/kotlin/base/scripting/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt
@@ -41,7 +41,7 @@ val loadScriptDefinitionsOnDemand
     get() = Registry.`is`("kotlin.scripting.load.definitions.on.demand", true)
 
 internal class LoadScriptDefinitionsStartupActivity : ProjectActivity {
-    override suspend fun execute(project: Project) : Unit = blockingContextScope {
+    override suspend fun execute(project: Project): Unit = blockingContextScope {
         if (loadScriptDefinitionsOnDemand) return@blockingContextScope
 
         if (isUnitTestMode()) {
@@ -143,9 +143,8 @@ open class ScriptDefinitionsManager(private val project: Project) : LazyScriptDe
 
         private val definitionsLock = ReentrantLock()
 
-        // @GuardedBy("definitionsLock")
         // Support for insertion order is crucial because 'getSources()' is based on EP order in XML (default configuration source goes last)
-        private val definitionsBySource = mutableMapOf<ScriptDefinitionsSource, List<ScriptDefinition>>()
+        private val definitionsBySource = ConcurrentHashMap<ScriptDefinitionsSource, List<ScriptDefinition>>()
 
         @Volatile
         private var definitions: List<ScriptDefinition>? = null
@@ -188,25 +187,29 @@ open class ScriptDefinitionsManager(private val project: Project) : LazyScriptDe
 
         override fun reloadDefinitionsBy(source: ScriptDefinitionsSource) = reloadDefinitionsInternal(listOf(source))
 
+        // This function is aimed to fix locks acquisition order.
+        // The internal block still may acquire the read lock, it just won't have an effect.
+        private fun withLocks(block: () -> Unit) = runReadAction { definitionsLock.withLock { block.invoke() } }
+
         private fun reloadDefinitionsInternal(sources: List<ScriptDefinitionsSource>): List<ScriptDefinition> {
             val scriptingSettings = kotlinScriptingSettingsSafe() ?: error("Kotlin script setting not found")
 
-            val loadedDefinitions: List<ScriptDefinition>?
+            var loadedDefinitions: List<ScriptDefinition>? = null
 
-            definitionsLock.withLock {
-                val (ms, newDefinitionsBySource) = measureTimeMillisWithResult {
-                    sources.associateWith {
-                        val (ms, definitions) = measureTimeMillisWithResult { it.safeGetDefinitions() }
-                        scriptingDebugLog { "Loaded definitions: time = $ms ms, source = ${it.javaClass.name}, definitions = ${definitions.map { it.name }}" }
-                        definitions
-                    }
+            val (ms, newDefinitionsBySource) = measureTimeMillisWithResult {
+                sources.associateWith {
+                    val (ms, definitions) = measureTimeMillisWithResult { it.safeGetDefinitions() /* can acquire read-action inside */ }
+                    scriptingDebugLog { "Loaded definitions: time = $ms ms, source = ${it.javaClass.name}, definitions = ${definitions.map { it.name }}" }
+                    definitions
                 }
+            }
 
-                scriptingDebugLog { "Definitions loading total time: $ms ms" }
+            scriptingDebugLog { "Definitions loading total time: $ms ms" }
 
-                if (newDefinitionsBySource.isEmpty())
-                    return emptyList()
+            if (newDefinitionsBySource.isEmpty())
+                return emptyList()
 
+            withLocks {
                 definitionsBySource.putAll(newDefinitionsBySource)
 
                 loadedDefinitions = definitionsBySource.values.flattenTo(mutableListOf())
@@ -214,8 +217,8 @@ open class ScriptDefinitionsManager(private val project: Project) : LazyScriptDe
                     .sortedBy(ScriptDefinition::order)
                     .takeIf { it.isNotEmpty() }
 
-                clearCache()
                 definitions = loadedDefinitions
+                clearCache()
             }
 
             activatedDefinitionSources.addAll(sources)
@@ -251,7 +254,7 @@ open class ScriptDefinitionsManager(private val project: Project) : LazyScriptDe
             val scriptingSettings = kotlinScriptingSettingsSafe() ?: return
             if (definitions == null) return
 
-            definitionsLock.withLock {
+            withLocks {
                 definitions?.let { list ->
                     list.forEach {
                         it.order = scriptingSettings.getScriptDefinitionOrder(it)