From f2dee2bf858ef7510ad337c84e479d2c005bf8e7 Mon Sep 17 00:00:00 2001 From: Simon Ogorodnik Date: Fri, 12 Aug 2022 23:06:54 +0200 Subject: [PATCH] Core. Fix data-race in ApplicationEnvironment initialization getOrCreateApplicationEnvironment is main entrypoint to create ApplicationEnvironment in production and compiler tests It is subject to be called concurrently, that's why APPLICATION_LOCK exists ApplicationEnvironment itself hosts Application from idea-core It is actually singleton, that is subject to be disposed, once all operations referencing it completes To properly dispose ApplicationEnvironment when there is no references left, we maintain reference counter aka ourProjectCount Originally, there was data-race caused by the fact, that ourProjectCount was updated after publication of application Linear, data-race occurs in following order T1: getOrCreateApplicationEnvironment returns application T2: Disposes its reference to application, causing ourProjectCount to reach zero, and disposing application that is already available to T1 T1: Updates counter, but its application already disposed --- .../jetbrains/kotlin/cli/common/Properties.kt | 11 +++++ .../cli/jvm/compiler/KotlinCoreEnvironment.kt | 47 +++++++------------ 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt b/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt index c9968142efe..0ef60f1ac2d 100644 --- a/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt +++ b/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt @@ -33,6 +33,17 @@ enum class CompilerSystemProperties(val property: String, val alwaysDirectAccess DAEMON_RMI_SOCKET_BACKLOG_SIZE_PROPERTY("kotlin.daemon.socket.backlog.size"), DAEMON_RMI_SOCKET_CONNECT_ATTEMPTS_PROPERTY("kotlin.daemon.socket.connect.attempts"), DAEMON_RMI_SOCKET_CONNECT_INTERVAL_PROPERTY("kotlin.daemon.socket.connect.interval"), + + /** + * Only specify that property if you want to avoid multiple initializations/disposals of KotlinCoreApplicationEnvironment + * + * **If not specified**, KotlinCoreApplicationEnvironment **WILL dispose** after all concurrent compilations finish, + * and will be re-initialized when starting new compilation + * + * If specified, KotlinCoreApplicationEnvironment will **NOT dispose** after all concurrent compilations finished + * and will be pinned with hard-reference + * **THIS LEAKS MEMORY** + * */ KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY("kotlin.environment.keepalive"), COMPILE_DAEMON_CUSTOM_RUN_FILES_PATH_FOR_TESTS("kotlin.daemon.custom.run.files.path.for.tests"), COMPILE_INCREMENTAL_WITH_ARTIFACT_TRANSFORM("kotlin.incremental.useClasspathSnapshot"), diff --git a/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/compiler/KotlinCoreEnvironment.kt b/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/compiler/KotlinCoreEnvironment.kt index b87ad9a4789..9c66efc2c32 100644 --- a/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/compiler/KotlinCoreEnvironment.kt +++ b/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/compiler/KotlinCoreEnvironment.kt @@ -443,9 +443,6 @@ class KotlinCoreEnvironment private constructor( val projectEnv = ProjectEnvironment(parentDisposable, appEnv, configuration) val environment = KotlinCoreEnvironment(projectEnv, configuration, configFiles) - synchronized(APPLICATION_LOCK) { - ourProjectCount++ - } return environment } @@ -453,15 +450,7 @@ class KotlinCoreEnvironment private constructor( fun createForProduction( projectEnvironment: ProjectEnvironment, configuration: CompilerConfiguration, configFiles: EnvironmentConfigFiles ): KotlinCoreEnvironment { - val environment = KotlinCoreEnvironment(projectEnvironment, configuration, configFiles) - - if (projectEnvironment.environment == applicationEnvironment) { - // accounting for core environment disposing - synchronized(APPLICATION_LOCK) { - ourProjectCount++ - } - } - return environment + return KotlinCoreEnvironment(projectEnvironment, configuration, configFiles) } @TestOnly @@ -506,14 +495,7 @@ class KotlinCoreEnvironment private constructor( ): KotlinCoreApplicationEnvironment { synchronized(APPLICATION_LOCK) { if (ourApplicationEnvironment == null) { - val disposable = if (unitTestMode) { - parentDisposable - } else { - // TODO this is a memory leak in the compiler, as this Disposable is not registered and thus is never disposed - // but using parentDisposable as disposable in compiler, causes access to application extension points after - // they was disposed, this needs to be fixed - Disposer.newDisposable("Disposable for the KotlinCoreApplicationEnvironment") - } + val disposable = Disposer.newDisposable("Disposable for the KotlinCoreApplicationEnvironment") ourApplicationEnvironment = createApplicationEnvironment(disposable, configuration, unitTestMode) ourProjectCount = 0 Disposer.register(disposable, Disposable { @@ -522,18 +504,21 @@ class KotlinCoreEnvironment private constructor( } }) } - // Disposing of the environment is unsafe in production then parallel builds are enabled, but turning it off universally - // breaks a lot of tests, therefore it is disabled for production and enabled for tests - if (CompilerSystemProperties.KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.value.toBooleanLenient() != true) { - // JPS may run many instances of the compiler in parallel (there's an option for compiling independent modules in parallel in IntelliJ) - // All projects share the same ApplicationEnvironment, and when the last project is disposed, the ApplicationEnvironment is disposed as well - Disposer.register(parentDisposable, Disposable { - synchronized(APPLICATION_LOCK) { - if (--ourProjectCount <= 0) { - disposeApplicationEnvironment() + try { + // Do not use this property unless you sure need it, causes Application to MEMORY LEAK + // Only valid use-case is when Application should be cached to avoid + // initialization costs + if (CompilerSystemProperties.KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.value.toBooleanLenient() != true) { + Disposer.register(parentDisposable, Disposable { + synchronized(APPLICATION_LOCK) { + if (--ourProjectCount <= 0) { + disposeApplicationEnvironment() + } } - } - }) + }) + } + } finally { + ourProjectCount++ } return ourApplicationEnvironment!!