From ae32eff54329f510d2caddb5c431edf7b14a22a8 Mon Sep 17 00:00:00 2001 From: Simon Ogorodnik Date: Tue, 28 Mar 2023 10:38:46 +0000 Subject: [PATCH] KT-57154: Fix JRT-FS contents served for wrong JDK home on JDK 9+ If the compiler runtime JDK is 9+, it will already contain JrtFileSystemProvider and won't use provided classloader In order to fix KT-57154 we need to provide "java.home" argument to newFileSystem In order to reduce the severity of the leak in KT-56789 we cache instances of FileSystem itself forever Otherwise, each invocation of newFileSystem on JDK 9+ will leak classloader, which is created deep inside the JDK code Add unit test for JRT-FS contents served through CoreJrtFs Add Gradle Integration test to test if the daemon correctly reads JDK contents from the specified toolchain and not from its runtime JDK ^KT-57154 Regression test for ^KT-57077 --- .../cli/jvm/modules/CoreJrtFileSystem.kt | 39 ++++++---- .../kotlin/test/jvm/compiler/CoreJrtFsTest.kt | 77 +++++++++++++++++++ .../kotlin/test/KotlinTestUtils.java | 3 + .../jdk/CustomJvmTargetOnJvmBaseTest.kt | 5 +- .../jetbrains/kotlin/gradle/KotlinDaemonIT.kt | 22 ++++++ .../onlyJdk11Compatible/build.gradle | 12 +++ .../src/main/kotlin/Main.kt | 17 ++++ 7 files changed, 158 insertions(+), 17 deletions(-) create mode 100644 compiler/tests-common-new/tests/org/jetbrains/kotlin/test/jvm/compiler/CoreJrtFsTest.kt create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/build.gradle create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/src/main/kotlin/Main.kt diff --git a/compiler/cli/cli-base/src/org/jetbrains/kotlin/cli/jvm/modules/CoreJrtFileSystem.kt b/compiler/cli/cli-base/src/org/jetbrains/kotlin/cli/jvm/modules/CoreJrtFileSystem.kt index dbbc4c0a146..335a4db84c4 100644 --- a/compiler/cli/cli-base/src/org/jetbrains/kotlin/cli/jvm/modules/CoreJrtFileSystem.kt +++ b/compiler/cli/cli-base/src/org/jetbrains/kotlin/cli/jvm/modules/CoreJrtFileSystem.kt @@ -20,31 +20,18 @@ import com.intellij.openapi.vfs.DeprecatedVirtualFileSystem import com.intellij.openapi.vfs.StandardFileSystems import com.intellij.openapi.vfs.VirtualFile import com.intellij.util.containers.ConcurrentFactoryMap -import com.intellij.util.containers.ContainerUtil import com.intellij.util.io.URLUtil import java.io.File import java.net.URI import java.net.URLClassLoader +import java.nio.file.FileSystem import java.nio.file.FileSystems // There's JrtFileSystem in idea-full which we can't use in the compiler because it depends on NewVirtualFileSystem, absent in intellij-core class CoreJrtFileSystem : DeprecatedVirtualFileSystem() { private val roots = ConcurrentFactoryMap.createMap { jdkHomePath -> - val jdkHome = File(jdkHomePath) - val rootUri = URI.create(StandardFileSystems.JRT_PROTOCOL + ":/") - val jrtFsJar = loadJrtFsJar(jdkHome) ?: return@createMap null - - /* - This ClassLoader actually lives as long as current thread due to ThreadLocal leak in jrt-fs, - See https://bugs.openjdk.java.net/browse/JDK-8260621 - So that cache allows us to avoid creating too many classloaders for same JDK and reduce severity of that leak - */ - val classLoader = globalJrtFsClassLoaderCache.computeIfAbsent(jrtFsJar) { - URLClassLoader(arrayOf(jrtFsJar.toURI().toURL()), null) - } - - val fileSystem = FileSystems.newFileSystem(rootUri, emptyMap(), classLoader) + val fileSystem = globalJrtFsCache[jdkHomePath] ?: return@createMap null CoreJrtVirtualFile(this, jdkHomePath, fileSystem.getPath(""), parent = null) } @@ -84,6 +71,26 @@ class CoreJrtFileSystem : DeprecatedVirtualFileSystem() { return Pair(localPath, pathInJar) } - private val globalJrtFsClassLoaderCache = ContainerUtil.createConcurrentWeakValueMap() + private val globalJrtFsCache = ConcurrentFactoryMap.createMap { jdkHomePath -> + val jdkHome = File(jdkHomePath) + val jrtFsJar = loadJrtFsJar(jdkHome) ?: return@createMap null + val rootUri = URI.create(StandardFileSystems.JRT_PROTOCOL + ":/") + /* + The ClassLoader, that was used to load JRT FS Provider actually lives as long as current thread due to ThreadLocal leak in jrt-fs, + See https://bugs.openjdk.java.net/browse/JDK-8260621 + So that cache allows us to avoid creating too many classloaders for same JDK and reduce severity of that leak + */ + if (isAtLeastJava9()) { + // If the runtime JDK is set to 9+ it has JrtFileSystemProvider, + // but to load proper jrt-fs (one that is pointed by jdkHome) we should provide "java.home" path + FileSystems.newFileSystem(rootUri, mapOf("java.home" to jdkHome.absolutePath)) + } else { + val classLoader = URLClassLoader(arrayOf(jrtFsJar.toURI().toURL()), null) + // If the runtime JDK is set to <9, there are no JrtFileSystemProvider, + // we should create classloader with jrt-fs.jar, and DO NOT NEED to pass "java.home" path, + // as otherwise it will incur additional classloader creation + FileSystems.newFileSystem(rootUri, emptyMap(), classLoader) + } + } } } diff --git a/compiler/tests-common-new/tests/org/jetbrains/kotlin/test/jvm/compiler/CoreJrtFsTest.kt b/compiler/tests-common-new/tests/org/jetbrains/kotlin/test/jvm/compiler/CoreJrtFsTest.kt new file mode 100644 index 00000000000..ed6263fba29 --- /dev/null +++ b/compiler/tests-common-new/tests/org/jetbrains/kotlin/test/jvm/compiler/CoreJrtFsTest.kt @@ -0,0 +1,77 @@ +/* + * Copyright 2010-2023 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. + */ + +package org.jetbrains.kotlin.test.jvm.compiler + +import com.intellij.openapi.Disposable +import com.intellij.openapi.util.Disposer +import com.intellij.openapi.vfs.VirtualFile +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.jetbrains.kotlin.config.CompilerConfiguration +import org.jetbrains.kotlin.config.JVMConfigurationKeys +import org.jetbrains.kotlin.test.KtAssert.assertEquals +import org.jetbrains.kotlin.test.TestJdkKind +import org.jetbrains.kotlin.test.services.configuration.JvmEnvironmentConfigurator +import org.jetbrains.org.objectweb.asm.ClassReader +import org.jetbrains.org.objectweb.asm.Opcodes +import org.jetbrains.org.objectweb.asm.tree.ClassNode +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test + +class CoreJrtFsTest { + + private fun checkClassVersion(expectedVersion: Int, actualClass: VirtualFile?) { + requireNotNull(actualClass) + val reader = ClassReader(actualClass.contentsToByteArray()) + + val node = ClassNode() + reader.accept(node, ClassReader.SKIP_CODE) + assertEquals( + "Expected class version($expectedVersion) differs in ${actualClass.path} (${node.version})", + expectedVersion, + node.version + ) + } + + lateinit var testRootDisposable: Disposable + + @BeforeEach + fun createRootDisposable() { + testRootDisposable = Disposable {} + } + + @AfterEach + fun disposeRootDisposable() { + Disposer.dispose(testRootDisposable) + } + + /** + * The test ensures that Thread class always contains version from JDK_11, when such javaHome is used + * Regardless of compiler runtime JDK + */ + @Test + fun testClassVersionsInJavaLangOfJdk11() { + val configuration = CompilerConfiguration() + val jdkHome = JvmEnvironmentConfigurator.getJdkHome(TestJdkKind.FULL_JDK_11) + requireNotNull(jdkHome) + configuration.put(JVMConfigurationKeys.JDK_HOME, jdkHome) + val environment = + KotlinCoreEnvironment.getOrCreateApplicationEnvironmentForTests(this.testRootDisposable, configuration) + + val jrt = environment.jrtFileSystem ?: error("No jrt-fs configured") + + val root = jrt.findFileByPath("$jdkHome!/modules/java.base/java/lang/") + requireNotNull(root) + val children = root.children.filter { it.extension == "class" } + assert(children.isNotEmpty()) + children.forEach { file -> + checkClassVersion( + Opcodes.V11, + file + ) + } + } +} \ No newline at end of file diff --git a/compiler/tests-common/tests/org/jetbrains/kotlin/test/KotlinTestUtils.java b/compiler/tests-common/tests/org/jetbrains/kotlin/test/KotlinTestUtils.java index 2e06f5b00d5..f5dd5416863 100644 --- a/compiler/tests-common/tests/org/jetbrains/kotlin/test/KotlinTestUtils.java +++ b/compiler/tests-common/tests/org/jetbrains/kotlin/test/KotlinTestUtils.java @@ -191,6 +191,9 @@ public class KotlinTestUtils { assert jdk6 != null : "Environment variable JDK_1_6 is not set"; configuration.put(JVMConfigurationKeys.JDK_HOME, new File(jdk6)); } + else if (jdkKind == TestJdkKind.FULL_JDK_11) { + configuration.put(JVMConfigurationKeys.JDK_HOME, KtTestUtil.getJdk11Home()); + } else if (jdkKind == TestJdkKind.FULL_JDK_17) { configuration.put(JVMConfigurationKeys.JDK_HOME, KtTestUtil.getJdk17Home()); } diff --git a/compiler/tests-different-jdk/tests/org/jetbrains/kotlin/codegen/jdk/CustomJvmTargetOnJvmBaseTest.kt b/compiler/tests-different-jdk/tests/org/jetbrains/kotlin/codegen/jdk/CustomJvmTargetOnJvmBaseTest.kt index 3d2a3cbe749..a868a1dd673 100644 --- a/compiler/tests-different-jdk/tests/org/jetbrains/kotlin/codegen/jdk/CustomJvmTargetOnJvmBaseTest.kt +++ b/compiler/tests-different-jdk/tests/org/jetbrains/kotlin/codegen/jdk/CustomJvmTargetOnJvmBaseTest.kt @@ -7,6 +7,7 @@ package org.jetbrains.kotlin.codegen.jdk +import org.jetbrains.kotlin.test.jvm.compiler.CoreJrtFsTest import org.jetbrains.kotlin.test.runners.codegen.* import org.junit.platform.runner.JUnitPlatform import org.junit.platform.suite.api.ExcludeTags @@ -26,7 +27,9 @@ import org.junit.runner.RunWith IrBlackBoxCodegenTestGenerated::class, IrBlackBoxInlineCodegenWithBytecodeInlinerTestGenerated::class, - IrCompileKotlinAgainstInlineKotlinTestGenerated::class + IrCompileKotlinAgainstInlineKotlinTestGenerated::class, + + CoreJrtFsTest::class ) @IncludeClassNamePatterns(".*Test.*Generated") @ExcludeTags("") diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KotlinDaemonIT.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KotlinDaemonIT.kt index 3e1f0c8e673..855b35c0055 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KotlinDaemonIT.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KotlinDaemonIT.kt @@ -154,6 +154,28 @@ class KotlinDaemonIT : KGPDaemonsBaseTest() { } } + @DisplayName("KT-57154: Compiler should use specified toolchain regardless of Gradle Runtime JDK") + @JdkVersions(versions = [JavaVersion.VERSION_1_8, JavaVersion.VERSION_11, JavaVersion.VERSION_17]) + @GradleWithJdkTest + @GradleTestVersions(minVersion = TestVersions.Gradle.MAX_SUPPORTED) + internal fun testCompilerRuntimeJdkToolchainIndependence(gradleVersion: GradleVersion, jdkVersion: JdkVersions.ProvidedJdk) { + project( + projectName = "kotlin-java-toolchain/onlyJdk11Compatible", + gradleVersion = gradleVersion, + buildJdk = jdkVersion.location, + buildOptions = defaultBuildOptions.copy(logLevel = LogLevel.INFO) + ) { + build("compileKotlin") { + val startOptions = output.findAllStringsPrefixed("starting the daemon as: ").single() + // ensure that new daemon was started and that specified JDK is used as runtime JDK for it + assert(startOptions.startsWith(jdkVersion.location.absolutePath)) { + printBuildOutput() + "Kotlin daemon used non-expected JDK (expected ${jdkVersion.location.absolutePath}): $startOptions" + } + } + } + } + private fun BuildResult.assertGradleClasspathNotLeaked() { assertOutputContains("Kotlin compiler classpath:") diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/build.gradle b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/build.gradle new file mode 100644 index 00000000000..848d5084f92 --- /dev/null +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/build.gradle @@ -0,0 +1,12 @@ +plugins { + id "org.jetbrains.kotlin.jvm" +} + +repositories { + mavenLocal() + mavenCentral() +} + +kotlin { + jvmToolchain(11) +} \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/src/main/kotlin/Main.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/src/main/kotlin/Main.kt new file mode 100644 index 00000000000..f37992aab36 --- /dev/null +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/kotlin-java-toolchain/onlyJdk11Compatible/src/main/kotlin/Main.kt @@ -0,0 +1,17 @@ +/* + * Copyright 2010-2023 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. + */ + + +/** + * A main function that could only compile on JDK 11-13, due to use of both new api and removed api + */ +fun main() { + // The function, that was only available until JDK 13 (https://bugs.openjdk.org/browse/JDK-8205131) + Runtime.getRuntime().traceInstructions(true) + + // The new overload for toArray that was added in JDK 11 (https://bugs.openjdk.org/browse/JDK-8060192) + val array: Array = listOf("").toArray { arrayOf("other") } +} +