From b1504e7f37a6c399b96bcae95d94b3a06603beed Mon Sep 17 00:00:00 2001 From: "Aleksei.Cherepanov" Date: Mon, 23 Sep 2024 12:05:14 +0000 Subject: [PATCH] [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 (cherry picked from commit 5f337f8d626f6ad49a2631322a3f536af7e386c5) Merge-request: KT-MR-18073 Merged-by: Aleksei Cherepanov --- .../kotlin/incremental/storage/BasicMap.kt | 5 +++ .../incremental/storage/BasicMapsOwner.kt | 42 ++++++++++--------- .../incremental/storage/InMemoryStorage.kt | 4 ++ .../kotlin/incremental/storage/LazyStorage.kt | 19 ++++++++- .../incremental/storage/PersistentStorage.kt | 7 ++++ .../incremental/CompilationTransactionTest.kt | 2 + 6 files changed, 58 insertions(+), 21 deletions(-) diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt index ebaa105187cba..90f69d989647c 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt @@ -166,6 +166,11 @@ abstract class AppendableSetBasicMap( fun append(key: KEY, elements: Set) { storage.append(key, elements) } + + @Synchronized + override fun clean() { + storage.clean() + } } abstract class BasicStringMap( 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 6b6d270cf7bb9..c2cfb04fc3539 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt +++ b/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 --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt index 0af8a0cbef337..4f3fcdf047f06 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt @@ -137,6 +137,10 @@ open class InMemoryStorage( storage.close() } + @Synchronized + override fun clean() { + storage.clean() + } } /** [InMemoryStorage] where a map entry's value is a [Collection] of elements of type [E]. */ diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt index e9be49c692a5a..20d224c36eb24 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt +++ b/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( @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 --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt index ab397a6d85d79..5833068d7ae8b 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt @@ -48,6 +48,8 @@ interface PersistentStorage : 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( 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 --git a/build-common/test/org/jetbrains/kotlin/incremental/CompilationTransactionTest.kt b/build-common/test/org/jetbrains/kotlin/incremental/CompilationTransactionTest.kt index a82f5e0de9971..4b362f6d495a6 100644 --- a/build-common/test/org/jetbrains/kotlin/incremental/CompilationTransactionTest.kt +++ b/build-common/test/org/jetbrains/kotlin/incremental/CompilationTransactionTest.kt @@ -50,6 +50,8 @@ private class InMemoryStorageWrapperMock : InMemoryStorageInterface { override fun get(key: Any) = null override fun contains(key: Any) = false + + override fun clean() {} } abstract class BaseCompilationTransactionTest {