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

build-common/src/org/jetbrains/kotlin/incremental/AbstractIncrementalCache.kt

@@ -176,8 +176,8 @@ abstract class AbstractIncrementalCache<ClassName>(
     protected class ClassFqNameToSourceMap(
         storageFile: File,
         private val pathConverter: FileToPathConverter
-    ) :
-        BasicStringMap<String>(storageFile, EnumeratorStringDescriptor(), PathStringDescriptor) {
+    ) : BasicStringMap<String>(storageFile, EnumeratorStringDescriptor(), PathStringDescriptor) {
+
         operator fun set(fqName: FqName, sourceFile: File) {
             storage[fqName.asString()] = pathConverter.toPath(sourceFile)
         }

build-common/src/org/jetbrains/kotlin/incremental/IncrementalJvmCache.kt

@@ -272,6 +272,7 @@ open class IncrementalJvmCache(
 
     private inner class ProtoMap(storageFile: File) : BasicStringMap<ProtoMapValue>(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<SerializedJavaClass>(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)
         }

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 <K, V, M : BasicMap<K, V>> 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<String, Exception>()
         maps.forEach {
@@ -66,5 +68,6 @@ open class BasicMapsOwner(val cachesDir: File) {
     }
 
     @TestOnly
+    @Synchronized
     fun dump(): String = maps.joinToString("\n\n") { it.dump() }
 }

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<K, V>(
     private val keyDescriptor: KeyDescriptor<K>,
     private val valueExternalizer: DataExternalizer<V>
 ) : LazyStorage<K, V> {
-    @Volatile
     private var storage: PersistentHashMap<K, V>? = null
 
     @Synchronized
@@ -80,8 +81,10 @@ class CachingLazyStorage<K, V>(
         try {
             storage?.close()
         } finally {
-            PersistentHashMap.deleteFilesStartingWith(storageFile)
             storage = null
+            if (!IOUtil.deleteAllFilesStartingWith(storageFile)) {
+                throw IOException("Could not delete internal storage: ${storageFile.absolutePath}")
+            }
         }
     }
 

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<Collection<String>>(storageFile, StringCollectionExternalizer) {
+internal open class ClassOneToManyMap(storageFile: File) : BasicStringMap<Collection<String>>(storageFile, StringCollectionExternalizer) {
     override fun dumpValue(value: Collection<String>): String = value.dumpCollection()
 
+    @Synchronized
     fun add(key: FqName, value: FqName) {
         storage.append(key.asString(), listOf(value.asString()))
     }
 
     operator fun get(key: FqName): Collection<FqName> =
-            storage[key.asString()]?.map(::FqName) ?: setOf()
+        storage[key.asString()]?.map(::FqName) ?: setOf()
 
+    @Synchronized
     operator fun set(key: FqName, values: Collection<FqName>) {
         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<FqName>) {
         val notRemoved = this[key].filter { it !in removed }
         this[key] = notRemoved

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<K, V>(
@@ -27,7 +29,6 @@ class NonCachingLazyStorage<K, V>(
     private val keyDescriptor: KeyDescriptor<K>,
     private val valueExternalizer: DataExternalizer<V>
 ) : LazyStorage<K, V> {
-    @Volatile
     private var storage: PersistentHashMap<K, V>? = null
 
     @Synchronized
@@ -76,11 +77,12 @@ class NonCachingLazyStorage<K, V>(
     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

compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCachesManager.kt

@@ -30,7 +30,12 @@ abstract class IncrementalCachesManager<PlatformCache : AbstractIncrementalCache
 ) {
     val pathConverter = IncrementalFileToPathConverter(rootProjectDir)
     private val caches = arrayListOf<BasicMapsOwner>()
+
+    var isClosed = false
+
+    @Synchronized
     protected fun <T : BasicMapsOwner> T.registerCache() {
+        assert(!isClosed) { "Attempted to add new cache into closed storage." }
         caches.add(this)
     }
 
@@ -41,9 +46,12 @@ abstract class IncrementalCachesManager<PlatformCache : AbstractIncrementalCache
     val lookupCache: LookupStorage = LookupStorage(lookupCacheDir, pathConverter).apply { registerCache() }
     abstract val platformCache: PlatformCache
 
+    @Synchronized
     fun close(flush: Boolean = false): Boolean {
+        if (isClosed) {
+            return true
+        }
         var successful = true
-
         for (cache in caches) {
             if (flush) {
                 try {
@@ -62,6 +70,7 @@ abstract class IncrementalCachesManager<PlatformCache : AbstractIncrementalCache
             }
         }
 
+        isClosed = true
         return successful
     }
 }

compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCompilerRunner.kt

@@ -37,6 +37,7 @@ import org.jetbrains.kotlin.incremental.util.BufferingMessageCollector
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.progress.CompilationCanceledStatus
 import java.io.File
+import java.io.IOException
 import java.util.*
 
 abstract class IncrementalCompilerRunner<
@@ -97,7 +98,10 @@ abstract class IncrementalCompilerRunner<
             val allKotlinFiles = allSourceFiles.filter { it.isKotlinFile(kotlinSourceFilesExtensions) }
             return compileIncrementally(args, caches, allKotlinFiles, CompilationMode.Rebuild(reason), messageCollector)
         }
-
+        // attempt IC
+        // if OK or failed compilation - return
+        // internal error - clear
+        // failed to close - clear
         return try {
             val changedFiles = providedChangedFiles ?: caches.inputsCache.sourceSnapshotMap.compareAndUpdate(allSourceFiles)
             val compilationMode = sourcesToCompile(caches, changedFiles, args, messageCollector)
@@ -143,8 +147,8 @@ abstract class IncrementalCompilerRunner<
             }
         }
 
-        assert(!destinationDir.exists()) { "Could not delete destination dir $destinationDir" }
-        assert(!workingDir.exists()) { "Could not delete caches dir $workingDir" }
+        if (destinationDir.exists()) throw IOException("Could not delete directory $destinationDir.")
+        if (workingDir.exists()) throw IOException("Could not delete internal caches in folder $workingDir")
         destinationDir.mkdirs()
         workingDir.mkdirs()
     }

compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/snapshots/FileSnapshotMap.kt

@@ -26,6 +26,7 @@ class FileSnapshotMap(storageFile: File) : BasicStringMap<FileSnapshot>(storageF
     override fun dumpValue(value: FileSnapshot): String =
         value.toString()
 
+    @Synchronized
     fun compareAndUpdate(newFiles: Iterable<File>): ChangedFiles.Known {
         val snapshotProvider = SimpleFileSnapshotProviderImpl()
         val newOrModified = ArrayList<File>()

compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMap.kt

@@ -12,6 +12,7 @@ class SourceToOutputFilesMap(
     storageFile: File
 ) : BasicStringMap<Collection<String>>(storageFile, PathStringDescriptor, StringCollectionExternalizer) {
 
+    @Synchronized
     operator fun set(sourceFile: File, outputFiles: Collection<File>) {
         storage[sourceFile.absolutePath] = outputFiles.map { it.absolutePath }
     }
@@ -22,6 +23,7 @@ class SourceToOutputFilesMap(
     override fun dumpValue(value: Collection<String>) =
         value.dumpCollection()
 
+    @Synchronized
     fun remove(file: File): Collection<File> =
         get(file).also { storage.remove(file.absolutePath) }
 }