From 275a02ce88e44f144bbb5f0718cfb223cd429051 Mon Sep 17 00:00:00 2001 From: Andrey Uskov Date: Thu, 12 Nov 2020 17:25:18 +0300 Subject: [PATCH] Fix synchronization when working with IC caches IC caches could be modified and read from different threads. In JPS builder these threads are RMI worker (invoked from Compiler Daemon) and JPS worker thread. Proper synchronization fixes cases when caches could become broken. #KT-42265 Fixed #KT-42194 Fixed #KT-42265 Fixed #KT-42937 Fixed --- .../kotlin/incremental/AbstractIncrementalCache.kt | 4 ++-- .../kotlin/incremental/IncrementalJvmCache.kt | 11 +++++++++++ .../kotlin/incremental/storage/BasicMapsOwner.kt | 3 +++ .../kotlin/incremental/storage/CachingLazyStorage.kt | 7 +++++-- .../kotlin/incremental/storage/ClassOneToManyMap.kt | 12 ++++++++---- .../incremental/storage/NonCachingLazyStorage.kt | 12 +++++++----- .../kotlin/incremental/IncrementalCachesManager.kt | 11 ++++++++++- .../kotlin/incremental/IncrementalCompilerRunner.kt | 10 +++++++--- .../kotlin/incremental/snapshots/FileSnapshotMap.kt | 1 + .../incremental/storage/SourceToOutputFilesMap.kt | 2 ++ 10 files changed, 56 insertions(+), 17 deletions(-) diff --git a/build-common/src/org/jetbrains/kotlin/incremental/AbstractIncrementalCache.kt b/build-common/src/org/jetbrains/kotlin/incremental/AbstractIncrementalCache.kt index 337c09af52b..c768fa3a8cb 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/AbstractIncrementalCache.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/AbstractIncrementalCache.kt @@ -176,8 +176,8 @@ abstract class AbstractIncrementalCache( protected class ClassFqNameToSourceMap( storageFile: File, private val pathConverter: FileToPathConverter - ) : - BasicStringMap(storageFile, EnumeratorStringDescriptor(), PathStringDescriptor) { + ) : BasicStringMap(storageFile, EnumeratorStringDescriptor(), PathStringDescriptor) { + operator fun set(fqName: FqName, sourceFile: File) { storage[fqName.asString()] = pathConverter.toPath(sourceFile) } diff --git a/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt b/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt index c5f01e1824b..84d75a417d6 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt @@ -272,6 +272,7 @@ open class IncrementalJvmCache( private inner class ProtoMap(storageFile: File) : BasicStringMap(storageFile, ProtoMapValueExternalizer) { + @Synchronized fun process(kotlinClass: LocalFileKotlinClass, changesCollector: ChangesCollector) { return put(kotlinClass, changesCollector) } @@ -283,10 +284,12 @@ open class IncrementalJvmCache( // from files compiled during last round. // However there is no need to compare old and new data in this case // (also that would fail with exception). + @Synchronized fun storeModuleMapping(className: JvmClassName, bytes: ByteArray) { storage[className.internalName] = ProtoMapValue(isPackageFacade = false, bytes = bytes, strings = emptyArray()) } + @Synchronized private fun put(kotlinClass: LocalFileKotlinClass, changesCollector: ChangesCollector) { val header = kotlinClass.classHeader @@ -309,6 +312,7 @@ open class IncrementalJvmCache( operator fun get(className: JvmClassName): ProtoMapValue? = storage[className.internalName] + @Synchronized fun remove(className: JvmClassName, changesCollector: ChangesCollector) { val key = className.internalName val oldValue = storage[key] ?: return @@ -325,6 +329,8 @@ open class IncrementalJvmCache( private inner class JavaSourcesProtoMap(storageFile: File) : BasicStringMap(storageFile, JavaClassProtoMapValueExternalizer) { + + @Synchronized fun process(jvmClassName: JvmClassName, newData: SerializedJavaClass, changesCollector: ChangesCollector) { val key = jvmClassName.internalName val oldData = storage[key] @@ -336,6 +342,7 @@ open class IncrementalJvmCache( ) } + @Synchronized fun remove(className: JvmClassName, changesCollector: ChangesCollector) { val key = className.internalName val oldValue = storage[key] ?: return @@ -375,6 +382,7 @@ open class IncrementalJvmCache( operator fun contains(className: JvmClassName): Boolean = className.internalName in storage + @Synchronized fun process(kotlinClass: LocalFileKotlinClass, changesCollector: ChangesCollector) { val key = kotlinClass.className.internalName val oldMap = storage[key] ?: emptyMap() @@ -391,6 +399,7 @@ open class IncrementalJvmCache( } } + @Synchronized fun remove(className: JvmClassName) { storage.remove(className.internalName) } @@ -523,6 +532,7 @@ open class IncrementalJvmCache( return result } + @Synchronized fun process(kotlinClass: LocalFileKotlinClass, changesCollector: ChangesCollector) { val key = kotlinClass.className.internalName val oldMap = storage[key] ?: emptyMap() @@ -548,6 +558,7 @@ open class IncrementalJvmCache( private fun functionNameBySignature(signature: String): String = signature.substringBefore("(") + @Synchronized fun remove(className: JvmClassName) { storage.remove(className.internalName) } diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt index 571f26bb968..57d90607c4f 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt @@ -30,6 +30,7 @@ open class BasicMapsOwner(val cachesDir: File) { protected val String.storageFile: File get() = File(cachesDir, this + "." + CACHE_EXTENSION) + @Synchronized protected fun > registerMap(map: M): M { maps.add(map) return map @@ -47,6 +48,7 @@ open class BasicMapsOwner(val cachesDir: File) { forEachMapSafe("flush") { it.flush(memoryCachesOnly) } } + @Synchronized private fun forEachMapSafe(actionName: String, action: (BasicMap<*, *>) -> Unit) { val actionExceptions = LinkedHashMap() maps.forEach { @@ -66,5 +68,6 @@ open class BasicMapsOwner(val cachesDir: File) { } @TestOnly + @Synchronized fun dump(): String = maps.joinToString("\n\n") { it.dump() } } \ No newline at end of file diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/CachingLazyStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/CachingLazyStorage.kt index 360a4e517b1..7e4a25c2ada 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/CachingLazyStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/CachingLazyStorage.kt @@ -17,9 +17,11 @@ package org.jetbrains.kotlin.incremental.storage import com.intellij.util.io.DataExternalizer +import com.intellij.util.io.IOUtil import com.intellij.util.io.KeyDescriptor import com.intellij.util.io.PersistentHashMap import java.io.File +import java.io.IOException /** @@ -30,7 +32,6 @@ class CachingLazyStorage( private val keyDescriptor: KeyDescriptor, private val valueExternalizer: DataExternalizer ) : LazyStorage { - @Volatile private var storage: PersistentHashMap? = null @Synchronized @@ -80,8 +81,10 @@ class CachingLazyStorage( try { storage?.close() } finally { - PersistentHashMap.deleteFilesStartingWith(storageFile) storage = null + if (!IOUtil.deleteAllFilesStartingWith(storageFile)) { + throw IOException("Could not delete internal storage: ${storageFile.absolutePath}") + } } } diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/ClassOneToManyMap.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/ClassOneToManyMap.kt index 6549c96224a..f06524591b6 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/ClassOneToManyMap.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/ClassOneToManyMap.kt @@ -20,18 +20,18 @@ import org.jetbrains.kotlin.incremental.dumpCollection import org.jetbrains.kotlin.name.FqName import java.io.File -internal open class ClassOneToManyMap( - storageFile: File -) : BasicStringMap>(storageFile, StringCollectionExternalizer) { +internal open class ClassOneToManyMap(storageFile: File) : BasicStringMap>(storageFile, StringCollectionExternalizer) { override fun dumpValue(value: Collection): String = value.dumpCollection() + @Synchronized fun add(key: FqName, value: FqName) { storage.append(key.asString(), listOf(value.asString())) } operator fun get(key: FqName): Collection = - storage[key.asString()]?.map(::FqName) ?: setOf() + storage[key.asString()]?.map(::FqName) ?: setOf() + @Synchronized operator fun set(key: FqName, values: Collection) { if (values.isEmpty()) { remove(key) @@ -41,10 +41,14 @@ internal open class ClassOneToManyMap( storage[key.asString()] = values.map(FqName::asString) } + @Synchronized fun remove(key: FqName) { storage.remove(key.asString()) } + // Access to caches could be done from multiple threads (e.g. JPS worker and RMI). The underlying collection is already synchronized, + // thus we need synchronization of this method and all modification methods. + @Synchronized fun removeValues(key: FqName, removed: Set) { val notRemoved = this[key].filter { it !in removed } this[key] = notRemoved diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/NonCachingLazyStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/NonCachingLazyStorage.kt index 614426e0b1e..755ea8262ec 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/NonCachingLazyStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/NonCachingLazyStorage.kt @@ -17,9 +17,11 @@ package org.jetbrains.kotlin.incremental.storage import com.intellij.util.io.DataExternalizer +import com.intellij.util.io.IOUtil import com.intellij.util.io.KeyDescriptor import com.intellij.util.io.PersistentHashMap import java.io.File +import java.io.IOException class NonCachingLazyStorage( @@ -27,7 +29,6 @@ class NonCachingLazyStorage( private val keyDescriptor: KeyDescriptor, private val valueExternalizer: DataExternalizer ) : LazyStorage { - @Volatile private var storage: PersistentHashMap? = null @Synchronized @@ -76,11 +77,12 @@ class NonCachingLazyStorage( override fun clean() { try { storage?.close() - } catch (ignored: Throwable) { + } finally { + storage = null + if (!IOUtil.deleteAllFilesStartingWith(storageFile)) { + throw IOException("Could not delete internal storage: ${storageFile.absolutePath}") + } } - - PersistentHashMap.deleteFilesStartingWith(storageFile) - storage = null } @Synchronized diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCachesManager.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCachesManager.kt index c529463ac24..83c8bb1f881 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCachesManager.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCachesManager.kt @@ -30,7 +30,12 @@ abstract class IncrementalCachesManager() + + var isClosed = false + + @Synchronized protected fun T.registerCache() { + assert(!isClosed) { "Attempted to add new cache into closed storage." } caches.add(this) } @@ -41,9 +46,12 @@ abstract class IncrementalCachesManager(storageF override fun dumpValue(value: FileSnapshot): String = value.toString() + @Synchronized fun compareAndUpdate(newFiles: Iterable): ChangedFiles.Known { val snapshotProvider = SimpleFileSnapshotProviderImpl() val newOrModified = ArrayList() diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMap.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMap.kt index 18448f56799..28032db9d44 100644 --- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMap.kt +++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMap.kt @@ -12,6 +12,7 @@ class SourceToOutputFilesMap( storageFile: File ) : BasicStringMap>(storageFile, PathStringDescriptor, StringCollectionExternalizer) { + @Synchronized operator fun set(sourceFile: File, outputFiles: Collection) { storage[sourceFile.absolutePath] = outputFiles.map { it.absolutePath } } @@ -22,6 +23,7 @@ class SourceToOutputFilesMap( override fun dumpValue(value: Collection) = value.dumpCollection() + @Synchronized fun remove(file: File): Collection = get(file).also { storage.remove(file.absolutePath) } } \ No newline at end of file