Refine locking to avoid dead lock

#KT-46215 Fixed
This commit is contained in:
Vladimir Dolzhenko
2021-04-21 11:01:12 +00:00
committed by Space
parent 6ae4c3f281
commit 590a8d088d
3 changed files with 48 additions and 22 deletions
@@ -76,6 +76,7 @@ class LoadScriptDefinitionsStartupActivity : StartupActivity {
} }
class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinitionProvider() { class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinitionProvider() {
private val definitionsLock = ReentrantLock()
private var definitionsBySource = mutableMapOf<ScriptDefinitionsSource, List<ScriptDefinition>>() private var definitionsBySource = mutableMapOf<ScriptDefinitionsSource, List<ScriptDefinition>>()
@Volatile @Volatile
@@ -129,12 +130,12 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit
fun reloadDefinitionsBy(source: ScriptDefinitionsSource) { fun reloadDefinitionsBy(source: ScriptDefinitionsSource) {
if (definitions == null) return // not loaded yet if (definitions == null) return // not loaded yet
lock.write { definitionsLock.withLock {
if (source !in definitionsBySource) error("Unknown script definition source: $source") if (source !in definitionsBySource) error("Unknown script definition source: $source")
} }
val safeGetDefinitions = source.safeGetDefinitions() val safeGetDefinitions = source.safeGetDefinitions()
lock.write { definitionsLock.withLock {
definitionsBySource[source] = safeGetDefinitions definitionsBySource[source] = safeGetDefinitions
definitions = definitionsBySource.values.flattenTo(mutableListOf()) 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() val newDefinitionsBySource = getSources().map { it to it.safeGetDefinitions() }.toMap()
lock.write { definitionsLock.withLock {
definitionsBySource.putAll(newDefinitionsBySource) definitionsBySource.putAll(newDefinitionsBySource)
definitions = definitionsBySource.values.flattenTo(mutableListOf()) definitions = definitionsBySource.values.flattenTo(mutableListOf())
@@ -184,11 +185,11 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit
val scriptingSettings = kotlinScriptingSettingsSafe() ?: return val scriptingSettings = kotlinScriptingSettingsSafe() ?: return
definitions?.forEach { definitions?.forEach {
val order = scriptingSettings.getScriptDefinitionOrder(it) val order = scriptingSettings.getScriptDefinitionOrder(it)
lock.write { definitionsLock.withLock {
it.order = order it.order = order
} }
} }
lock.write { definitionsLock.withLock {
definitions = definitions?.sortedBy { it.order } definitions = definitions?.sortedBy { it.order }
updateDefinitions() updateDefinitions()
@@ -206,7 +207,7 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit
fun isReady(): Boolean { fun isReady(): Boolean {
if (definitions == null) return false if (definitions == null) return false
val keys = lock.write { definitionsBySource.keys } val keys = definitionsLock.withLock { definitionsBySource.keys }
return keys.all { source -> return keys.all { source ->
// TODO: implement another API for readiness checking // TODO: implement another API for readiness checking
(source as? ScriptDefinitionContributor)?.isReady() != false (source as? ScriptDefinitionContributor)?.isReady() != false
@@ -220,7 +221,7 @@ class ScriptDefinitionsManager(private val project: Project) : LazyScriptDefinit
} }
private fun updateDefinitions() { 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 if (project.isDisposed) return
val fileTypeManager = FileTypeManager.getInstance() val fileTypeManager = FileTypeManager.getInstance()
@@ -8,8 +8,10 @@ package org.jetbrains.kotlin.scripting.definitions
import com.intellij.ide.highlighter.JavaFileType import com.intellij.ide.highlighter.JavaFileType
import org.jetbrains.kotlin.idea.KotlinFileType import org.jetbrains.kotlin.idea.KotlinFileType
import java.net.URI import java.net.URI
import java.util.concurrent.locks.ReentrantLock
import java.util.concurrent.locks.ReentrantReadWriteLock import java.util.concurrent.locks.ReentrantReadWriteLock
import kotlin.concurrent.read import kotlin.concurrent.read
import kotlin.concurrent.withLock
import kotlin.concurrent.write import kotlin.concurrent.write
import kotlin.script.experimental.api.SourceCode import kotlin.script.experimental.api.SourceCode
import kotlin.script.experimental.host.ScriptingHostConfiguration import kotlin.script.experimental.host.ScriptingHostConfiguration
@@ -17,7 +19,10 @@ import kotlin.script.experimental.jvm.defaultJvmScriptingHostConfiguration
abstract class LazyScriptDefinitionProvider : ScriptDefinitionProvider { abstract class LazyScriptDefinitionProvider : ScriptDefinitionProvider {
protected val lock = ReentrantReadWriteLock() @Volatile
private var disposed: Boolean = false
private val cachedDefinitionsLock = ReentrantLock()
protected abstract val currentDefinitions: Sequence<ScriptDefinition> protected abstract val currentDefinitions: Sequence<ScriptDefinition>
@@ -28,45 +33,63 @@ abstract class LazyScriptDefinitionProvider : ScriptDefinitionProvider {
protected val fixedDefinitions: HashMap<URI, ScriptDefinition> = HashMap() protected val fixedDefinitions: HashMap<URI, ScriptDefinition> = HashMap()
@Volatile
private var _cachedDefinitions: Sequence<ScriptDefinition>? = null private var _cachedDefinitions: Sequence<ScriptDefinition>? = null
private val cachedDefinitions: Sequence<ScriptDefinition> private val cachedDefinitions: Sequence<ScriptDefinition>
get() { get() {
assert(lock.readLockCount > 0) { "cachedDefinitions should only be used under the read lock" } return _cachedDefinitions ?: run {
if (_cachedDefinitions == null) lock.write { assert(cachedDefinitionsLock.holdCount == 0) { "cachedDefinitions should not be used under the lock" }
_cachedDefinitions = CachingSequence(currentDefinitions.constrainOnce()) val originalSequence = currentDefinitions.constrainOnce()
cachedDefinitionsLock.withLock {
_cachedDefinitions ?: run {
if (!disposed) {
val seq = CachingSequence(originalSequence)
_cachedDefinitions = seq
seq
} else {
emptySequence()
}
}
}
} }
return _cachedDefinitions!!
} }
protected fun clearCache() { protected fun clearCache() {
lock.write { cachedDefinitionsLock.withLock {
_cachedDefinitions = null _cachedDefinitions = null
} }
} }
protected fun dispose() {
disposed = true
clearCache()
}
protected open fun nonScriptId(locationId: String): Boolean = protected open fun nonScriptId(locationId: String): Boolean =
nonScriptFilenameSuffixes.any { nonScriptFilenameSuffixes.any {
locationId.endsWith(it, ignoreCase = true) locationId.endsWith(it, ignoreCase = true)
} }
override fun findDefinition(script: SourceCode): ScriptDefinition? = override fun findDefinition(script: SourceCode): ScriptDefinition? =
if (script.locationId == null || nonScriptId(script.locationId!!)) null if (script.locationId == null || nonScriptId(script.locationId!!)) {
else lock.read { null
} else {
cachedDefinitions.firstOrNull { it.isScript(script) } cachedDefinitions.firstOrNull { it.isScript(script) }
} }
@Suppress("OverridingDeprecatedMember", "DEPRECATION") @Suppress("OverridingDeprecatedMember", "DEPRECATION")
override fun findScriptDefinition(fileName: String): KotlinScriptDefinition? = override fun findScriptDefinition(fileName: String): KotlinScriptDefinition? =
if (nonScriptId(fileName)) null if (nonScriptId(fileName)) {
else lock.read { null
} else {
cachedDefinitions.map { it.legacyDefinition }.firstOrNull { it.isScript(fileName) } cachedDefinitions.map { it.legacyDefinition }.firstOrNull { it.isScript(fileName) }
} }
override fun isScript(script: SourceCode): Boolean = findDefinition(script) != null override fun isScript(script: SourceCode): Boolean = findDefinition(script) != null
override fun getKnownFilenameExtensions(): Sequence<String> = lock.read { override fun getKnownFilenameExtensions(): Sequence<String> =
cachedDefinitions.map { it.fileExtension } cachedDefinitions.map { it.fileExtension }
}
@Suppress("OverridingDeprecatedMember", "DEPRECATION") @Suppress("OverridingDeprecatedMember", "DEPRECATION")
override fun getDefaultScriptDefinition(): KotlinScriptDefinition = getDefaultDefinition().legacyDefinition override fun getDefaultScriptDefinition(): KotlinScriptDefinition = getDefaultDefinition().legacyDefinition
@@ -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.LazyScriptDefinitionProvider
import org.jetbrains.kotlin.scripting.definitions.ScriptDefinition import org.jetbrains.kotlin.scripting.definitions.ScriptDefinition
import org.jetbrains.kotlin.scripting.definitions.ScriptDefinitionsSource 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() { open class CliScriptDefinitionProvider : LazyScriptDefinitionProvider() {
private val definitionsLock = ReentrantLock()
private val definitionsFromSources: MutableList<Sequence<ScriptDefinition>> = arrayListOf() private val definitionsFromSources: MutableList<Sequence<ScriptDefinition>> = arrayListOf()
private val definitions: MutableList<ScriptDefinition> = arrayListOf() private val definitions: MutableList<ScriptDefinition> = arrayListOf()
private var defaultDefinition: ScriptDefinition? = null private var defaultDefinition: ScriptDefinition? = null
@@ -22,7 +24,7 @@ open class CliScriptDefinitionProvider : LazyScriptDefinitionProvider() {
} }
fun setScriptDefinitions(newDefinitions: List<ScriptDefinition>) { fun setScriptDefinitions(newDefinitions: List<ScriptDefinition>) {
lock.write { definitionsLock.withLock {
definitions.clear() definitions.clear()
val (withoutStdDef, stdDef) = newDefinitions.partition { !it.isDefault } val (withoutStdDef, stdDef) = newDefinitions.partition { !it.isDefault }
definitions.addAll(withoutStdDef) definitions.addAll(withoutStdDef)
@@ -33,7 +35,7 @@ open class CliScriptDefinitionProvider : LazyScriptDefinitionProvider() {
} }
fun setScriptDefinitionsSources(newSources: List<ScriptDefinitionsSource>) { fun setScriptDefinitionsSources(newSources: List<ScriptDefinitionsSource>) {
lock.write { definitionsLock.withLock {
definitionsFromSources.clear() definitionsFromSources.clear()
for (it in newSources) { for (it in newSources) {
definitionsFromSources.add(it.definitions.constrainOnce()) definitionsFromSources.add(it.definitions.constrainOnce())