From 590a8d088d38a314bb2fa7d5949679a6af21f145 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Wed, 21 Apr 2021 11:01:12 +0000 Subject: [PATCH] Refine locking to avoid dead lock #KT-46215 Fixed --- .../core/script/ScriptDefinitionsManager.kt | 15 +++--- .../LazyScriptDefinitionProvider.kt | 47 ++++++++++++++----- .../CliScriptDefinitionProvider.kt | 8 ++-- 3 files changed, 48 insertions(+), 22 deletions(-) diff --git a/idea/idea-core/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt b/idea/idea-core/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt index 097915a6597..17461827c69 100644 --- a/idea/idea-core/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt +++ b/idea/idea-core/src/org/jetbrains/kotlin/idea/core/script/ScriptDefinitionsManager.kt @@ -76,6 +76,7 @@ class LoadScriptDefinitionsStartupActivity : StartupActivity { } class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinitionProvider() { + private val definitionsLock = ReentrantLock() private var definitionsBySource = mutableMapOf>() @Volatile @@ -129,12 +130,12 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit fun reloadDefinitionsBy(source: ScriptDefinitionsSource) { if (definitions == null) return // not loaded yet - lock.write { + definitionsLock.withLock { if (source !in definitionsBySource) error("Unknown script definition source: $source") } val safeGetDefinitions = source.safeGetDefinitions() - lock.write { + definitionsLock.withLock { definitionsBySource[source] = safeGetDefinitions definitions = definitionsBySource.values.flattenTo(mutableListOf()) @@ -172,7 +173,7 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit val newDefinitionsBySource = getSources().map { it to it.safeGetDefinitions() }.toMap() - lock.write { + definitionsLock.withLock { definitionsBySource.putAll(newDefinitionsBySource) definitions = definitionsBySource.values.flattenTo(mutableListOf()) @@ -184,11 +185,11 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit val scriptingSettings = kotlinScriptingSettingsSafe() ?: return definitions?.forEach { val order = scriptingSettings.getScriptDefinitionOrder(it) - lock.write { + definitionsLock.withLock { it.order = order } } - lock.write { + definitionsLock.withLock { definitions = definitions?.sortedBy { it.order } updateDefinitions() @@ -206,7 +207,7 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit fun isReady(): Boolean { if (definitions == null) return false - val keys = lock.write { definitionsBySource.keys } + val keys = definitionsLock.withLock { definitionsBySource.keys } return keys.all { source -> // TODO: implement another API for readiness checking (source as? ScriptDefinitionContributor)?.isReady() != false @@ -220,7 +221,7 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit } private fun updateDefinitions() { - assert(lock.isWriteLocked) { "updateDefinitions should only be called under the write lock" } + assert(definitionsLock.isLocked) { "updateDefinitions should only be called under the lock" } if (project.isDisposed) return val fileTypeManager = FileTypeManager.getInstance() diff --git a/plugins/scripting/scripting-compiler-impl/src/org/jetbrains/kotlin/scripting/definitions/LazyScriptDefinitionProvider.kt b/plugins/scripting/scripting-compiler-impl/src/org/jetbrains/kotlin/scripting/definitions/LazyScriptDefinitionProvider.kt index 34c984fb690..8dca772ee5f 100644 --- a/plugins/scripting/scripting-compiler-impl/src/org/jetbrains/kotlin/scripting/definitions/LazyScriptDefinitionProvider.kt +++ b/plugins/scripting/scripting-compiler-impl/src/org/jetbrains/kotlin/scripting/definitions/LazyScriptDefinitionProvider.kt @@ -8,8 +8,10 @@ package org.jetbrains.kotlin.scripting.definitions import com.intellij.ide.highlighter.JavaFileType import org.jetbrains.kotlin.idea.KotlinFileType import java.net.URI +import java.util.concurrent.locks.ReentrantLock import java.util.concurrent.locks.ReentrantReadWriteLock import kotlin.concurrent.read +import kotlin.concurrent.withLock import kotlin.concurrent.write import kotlin.script.experimental.api.SourceCode import kotlin.script.experimental.host.ScriptingHostConfiguration @@ -17,7 +19,10 @@ import kotlin.script.experimental.jvm.defaultJvmScriptingHostConfiguration abstract class LazyScriptDefinitionProvider : ScriptDefinitionProvider { - protected val lock = ReentrantReadWriteLock() + @Volatile + private var disposed: Boolean = false + + private val cachedDefinitionsLock = ReentrantLock() protected abstract val currentDefinitions: Sequence @@ -28,45 +33,63 @@ abstract class LazyScriptDefinitionProvider : ScriptDefinitionProvider { protected val fixedDefinitions: HashMap = HashMap() + @Volatile private var _cachedDefinitions: Sequence? = null + private val cachedDefinitions: Sequence get() { - assert(lock.readLockCount > 0) { "cachedDefinitions should only be used under the read lock" } - if (_cachedDefinitions == null) lock.write { - _cachedDefinitions = CachingSequence(currentDefinitions.constrainOnce()) + return _cachedDefinitions ?: run { + assert(cachedDefinitionsLock.holdCount == 0) { "cachedDefinitions should not be used under the lock" } + val originalSequence = currentDefinitions.constrainOnce() + cachedDefinitionsLock.withLock { + _cachedDefinitions ?: run { + if (!disposed) { + val seq = CachingSequence(originalSequence) + _cachedDefinitions = seq + seq + } else { + emptySequence() + } + } + } } - return _cachedDefinitions!! } protected fun clearCache() { - lock.write { + cachedDefinitionsLock.withLock { _cachedDefinitions = null } } + protected fun dispose() { + disposed = true + clearCache() + } + protected open fun nonScriptId(locationId: String): Boolean = nonScriptFilenameSuffixes.any { locationId.endsWith(it, ignoreCase = true) } override fun findDefinition(script: SourceCode): ScriptDefinition? = - if (script.locationId == null || nonScriptId(script.locationId!!)) null - else lock.read { + if (script.locationId == null || nonScriptId(script.locationId!!)) { + null + } else { cachedDefinitions.firstOrNull { it.isScript(script) } } @Suppress("OverridingDeprecatedMember", "DEPRECATION") override fun findScriptDefinition(fileName: String): KotlinScriptDefinition? = - if (nonScriptId(fileName)) null - else lock.read { + if (nonScriptId(fileName)) { + null + } else { cachedDefinitions.map { it.legacyDefinition }.firstOrNull { it.isScript(fileName) } } override fun isScript(script: SourceCode): Boolean = findDefinition(script) != null - override fun getKnownFilenameExtensions(): Sequence = lock.read { + override fun getKnownFilenameExtensions(): Sequence = cachedDefinitions.map { it.fileExtension } - } @Suppress("OverridingDeprecatedMember", "DEPRECATION") override fun getDefaultScriptDefinition(): KotlinScriptDefinition = getDefaultDefinition().legacyDefinition diff --git a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/definitions/CliScriptDefinitionProvider.kt b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/definitions/CliScriptDefinitionProvider.kt index bbeb37d7d21..3bd5f77410a 100644 --- a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/definitions/CliScriptDefinitionProvider.kt +++ b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/definitions/CliScriptDefinitionProvider.kt @@ -8,9 +8,11 @@ package org.jetbrains.kotlin.scripting.compiler.plugin.definitions import org.jetbrains.kotlin.scripting.definitions.LazyScriptDefinitionProvider import org.jetbrains.kotlin.scripting.definitions.ScriptDefinition import org.jetbrains.kotlin.scripting.definitions.ScriptDefinitionsSource -import kotlin.concurrent.write +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock open class CliScriptDefinitionProvider : LazyScriptDefinitionProvider() { + private val definitionsLock = ReentrantLock() private val definitionsFromSources: MutableList> = arrayListOf() private val definitions: MutableList = arrayListOf() private var defaultDefinition: ScriptDefinition? = null @@ -22,7 +24,7 @@ open class CliScriptDefinitionProvider : LazyScriptDefinitionProvider() { } fun setScriptDefinitions(newDefinitions: List) { - lock.write { + definitionsLock.withLock { definitions.clear() val (withoutStdDef, stdDef) = newDefinitions.partition { !it.isDefault } definitions.addAll(withoutStdDef) @@ -33,7 +35,7 @@ open class CliScriptDefinitionProvider : LazyScriptDefinitionProvider() { } fun setScriptDefinitionsSources(newSources: List) { - lock.write { + definitionsLock.withLock { definitionsFromSources.clear() for (it in newSources) { definitionsFromSources.add(it.definitions.constrainOnce())