[IC] Add synchronized clean methods to storage classes

Return synchronized clean methods across various storage classes to ensure proper cleanup and avoid race conditions. This change is critical for preventing "Storage already closed" exceptions during JPS builds.

^KTIJ-31276 Fixed

Merge-request: KT-MR-17937
Merged-by: Aleksei Cherepanov <[email protected]>
(cherry picked from commit 5f337f8)


Merge-request: KT-MR-18073
Merged-by: Aleksei Cherepanov <[email protected]>

Author: Aleksei.Cherepanov

Diff for: build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt

@@ -166,6 +166,11 @@ abstract class AppendableSetBasicMap<KEY, E>(
     fun append(key: KEY, elements: Set<E>) {
         storage.append(key, elements)
     }
+
+    @Synchronized
+    override fun clean() {
+        storage.clean()
+    }
 }
 
 abstract class BasicStringMap<VALUE>(

Diff for: build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt

@@ -41,40 +41,42 @@ open class BasicMapsOwner(val cachesDir: File) : Closeable {
         forEachMapSafe("flush", BasicMap<*, *>::flush)
     }
 
-    override fun close() {
-        forEachMapSafe("close", BasicMap<*, *>::close)
-    }
-
     open fun deleteStorageFiles() {
         forEachMapSafe("deleteStorageFiles", BasicMap<*, *>::deleteStorageFiles)
     }
 
     /**
-     * DEPRECATED: This API should be removed because
-     *   - It's not clear what [memoryCachesOnly] means.
-     *   - In the past, when `memoryCachesOnly=true` we applied a small optimization: Checking
-     *   [com.intellij.util.io.PersistentHashMap.isDirty] before calling [com.intellij.util.io.PersistentHashMap.force]. However, if that
-     *   optimization is useful, it's better to always do it (perhaps inside the [com.intellij.util.io.PersistentHashMap.force] method
-     *   itself) rather than doing it based on the value of this parameter.
-     *
-     * Instead, just call [flush] (without a parameter) directly.
+     * Please do not remove or modify this function.
+     * It is implementing [org.jetbrains.jps.incremental.storage.StorageOwner] interface and needed for correct JPS compilation.
+     */
+    override fun close() {
+        forEachMapSafe("close", BasicMap<*, *>::close)
+    }
+
+    /**
+     * Please do not remove or modify this function.
+     * It is implementing [org.jetbrains.jps.incremental.storage.StorageOwner] interface and needed for correct JPS compilation.
      */
     fun flush(@Suppress("UNUSED_PARAMETER") memoryCachesOnly: Boolean) {
         flush()
     }
 
     /**
-     * DEPRECATED: This API should be removed because:
-     *   - It's not obvious what "clean" means: It does not exactly describe the current implementation, and it also sounds similar to
-     *   "clear" which means removing all the map entries, but this method does not do that.
-     *   - This method currently calls [close] (and [deleteStorageFiles]). However, [close] is often already called separately and
-     *   automatically, so this API makes it more likely for [close] to be accidentally called twice.
+     * Please do not remove or modify this function.
+     * It is implementing [org.jetbrains.jps.incremental.storage.StorageOwner] interface and needed for correct JPS compilation.
+     * Calling [org.jetbrains.kotlin.incremental.storage.BasicMapsOwner.close] here is unnecessary and will produce race conditions
+     * for JPS build because JPS can modify caches of dependant modules.
      *
-     * Instead, just call [close] and/or [deleteStorageFiles] explicitly.
+     * More context:
+     * 1) While compiling module Foo, thread A can open caches of dependent module Bar
+     * 2) When we will compile module Bar in thread B we can decide to rebuild the module (e.g. configuration of facet changed)
+     * 3) Thread B will call `clean` action on caches of module Bar
+     * 4) If `clean` action also call `close` action,
+     * it will close opened map and will make it unusable when it tries to add info after recompilation,
+     * which will cause a "Storage already closed" exception.
      */
     fun clean() {
-        close()
-        deleteStorageFiles()
+        forEachMapSafe("clean", BasicMap<*, *>::clean)
     }
 
     @Synchronized

Diff for: build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt

@@ -137,6 +137,10 @@ open class InMemoryStorage<KEY, VALUE>(
         storage.close()
     }
 
+    @Synchronized
+    override fun clean() {
+        storage.clean()
+    }
 }
 
 /** [InMemoryStorage] where a map entry's value is a [Collection] of elements of type [E]. */

Diff for: build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt

@@ -19,6 +19,7 @@ package org.jetbrains.kotlin.incremental.storage
 import com.intellij.util.CommonProcessors
 import com.intellij.util.io.AppendablePersistentMap
 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 org.jetbrains.kotlin.incremental.IncrementalCompilationContext
@@ -27,6 +28,7 @@ import java.io.DataInput
 import java.io.DataInputStream
 import java.io.DataOutput
 import java.io.File
+import java.io.IOException
 
 /**
  * [PersistentStorage] which delegates operations to a [PersistentHashMap]. Note that the [PersistentHashMap] is created lazily (only when
@@ -90,9 +92,24 @@ open class LazyStorage<KEY, VALUE>(
 
     @Synchronized
     override fun close() {
-        storage?.close()
+        try {
+            storage?.close()
+        } finally {
+            storage = null
+        }
     }
 
+    @Synchronized
+    override fun clean() {
+        try {
+            storage?.close()
+        } finally {
+            storage = null
+            if (!IOUtil.deleteAllFilesStartingWith(storageFile)) {
+                throw IOException("Could not delete internal storage: ${storageFile.absolutePath}")
+            }
+        }
+    }
 }
 
 /** [LazyStorage] where a map entry's value is a [Collection] of elements of type [E]. */

Diff for: build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt

@@ -48,6 +48,8 @@ interface PersistentStorage<KEY, VALUE> : Closeable {
 
     /** Writes any remaining in-memory changes to [storageFile] ([flush]) and closes this map. */
     override fun close()
+
+    fun clean()
 }
 
 /** [PersistentStorage] where a map entry's value is a [Collection] of elements of type [E]. */
@@ -108,6 +110,11 @@ abstract class PersistentStorageWrapper<KEY, VALUE>(
     override fun close() {
         storage.close()
     }
+
+    @Synchronized
+    override fun clean() {
+        storage.clean()
+    }
 }
 
 /** [PersistentStorageWrapper] where a map entry's value is a [Collection] of elements of type [E]. */

Diff for: build-common/test/org/jetbrains/kotlin/incremental/CompilationTransactionTest.kt

@@ -50,6 +50,8 @@ private class InMemoryStorageWrapperMock : InMemoryStorageInterface<Any, Any> {
     override fun get(key: Any) = null
 
     override fun contains(key: Any) = false
+
+    override fun clean() {}
 }
 
 abstract class BaseCompilationTransactionTest {