Reflection: fix exceptions on concurrent access to some properties

Using `ReflectionProperties.lazy` is incorrect because it allows several
threads to observe different resulting values if they're computing it
simultaneously (unlike `lazy(PUBLICATION)`, which always returns the
value that "won the race").

In the case of property delegates, for example, if we're invoking
`isAccessible = true` and then `getDelegate()` concurrently, it might
happen that when some thread invokes `getDelegate()`, it gets the
underlying Field object which was written by another thread and which
has not yet been made accessible, leading to
IllegalPropertyDelegateAccessException.

 #KT-27585 Fixed

.space/CODEOWNERS

@@ -164,7 +164,7 @@
 /compiler/testData/psi/ "Kotlin Compiler Core"
 /compiler/testData/psiUtil/ "Kotlin Compiler Core"
 /compiler/testData/recursiveProcessor/ "Kotlin Compiler Core"
-/compiler/testData/reflection/classLoaderForBuiltIns/ "Kotlin Libraries"
+/compiler/testData/reflection/ "Kotlin Libraries"
 /compiler/testData/repl/ "Kotlin Compiler Core"
 /compiler/testData/resolve/ "Kotlin Compiler Core"
 /compiler/testData/resolveAnnotations/ "Kotlin Compiler Core"

compiler/testData/reflection/concurrentAccessToPrivateFunction/test.kt

@@ -0,0 +1,38 @@
+import java.util.concurrent.atomic.AtomicInteger
+import java.util.concurrent.atomic.AtomicReference
+import java.util.concurrent.CyclicBarrier
+import kotlin.concurrent.thread
+import kotlin.reflect.full.*
+import kotlin.reflect.jvm.*
+
+const val N_THREADS = 50
+
+class C {
+    private fun function() {}
+}
+
+fun main() {
+    val instance = C()
+    val reference = C::class.functions.single { it.name == "function" }
+
+    val gate = CyclicBarrier(N_THREADS + 1)
+    var fail = AtomicReference<Throwable?>(null)
+    var finished = AtomicInteger(0)
+    for (i in 0 until N_THREADS) {
+        thread {
+            gate.await()
+            reference.isAccessible = true
+            try {
+                reference.javaMethod!!.invoke(instance)
+            } catch (e: Throwable) {
+                fail.set(e)
+            }
+            finished.incrementAndGet()
+        }
+    }
+
+    gate.await()
+
+    while (finished.get() != N_THREADS) Thread.sleep(25L)
+    fail.get()?.let { throw it }
+}

compiler/testData/reflection/concurrentAccessToPropertyDelegate/test.kt

@@ -0,0 +1,39 @@
+import java.util.concurrent.atomic.AtomicInteger
+import java.util.concurrent.atomic.AtomicReference
+import java.util.concurrent.CyclicBarrier
+import kotlin.concurrent.thread
+import kotlin.reflect.jvm.*
+
+const val N_THREADS = 50
+
+class Delegate {
+    operator fun getValue(x: Any?, y: Any?): String = "OK"
+    operator fun setValue(x: Any?, y: Any?, z: String) {}
+}
+
+var property by Delegate()
+
+fun main() {
+    val reference = ::property
+
+    val gate = CyclicBarrier(N_THREADS + 1)
+    var fail = AtomicReference<Throwable?>(null)
+    var finished = AtomicInteger(0)
+    for (i in 0 until N_THREADS) {
+        thread {
+            gate.await()
+            reference.isAccessible = true
+            try {
+                reference.getDelegate()!!
+            } catch (e: Throwable) {
+                fail.set(e)
+            }
+            finished.incrementAndGet()
+        }
+    }
+
+    gate.await()
+
+    while (finished.get() != N_THREADS) Thread.sleep(25L)
+    fail.get()?.let { throw it }
+}

compiler/tests/org/jetbrains/kotlin/reflection/ReflectionIntegrationTest.kt

@@ -34,25 +34,15 @@ class ReflectionIntegrationTest : KtUsefulTestCase() {
 
         val lib = CompilerTestUtil.compileJvmLibrary(File("$root/test.kt"))
 
-        val javaHome = System.getProperty("java.home")
+        runJava(
-        val javaExe = File(javaHome, "bin/java.exe").takeIf(File::exists)
-            ?: File(javaHome, "bin/java").takeIf(File::exists)
-            ?: error("Can't find 'java' executable in $javaHome")
-
-        val command = arrayOf(
-            javaExe.absolutePath,
             "-ea",
             "-classpath",
             tmpdir.absolutePath,
             "Main",
             lib.absolutePath,
             ForTestCompileRuntime.runtimeJarForTests().absolutePath,
-            ForTestCompileRuntime.reflectJarForTests().absolutePath
+            ForTestCompileRuntime.reflectJarForTests().absolutePath,
         )
-
-        val process = ProcessBuilder(*command).inheritIO().start()
-        process.waitFor(1, TimeUnit.MINUTES)
-        assertEquals(0, process.exitValue())
     }
 
     // This test checks that simultaneous access to kotlin-reflect from different threads works, in case the URLClassLoader instance is
@@ -95,4 +85,41 @@ class ReflectionIntegrationTest : KtUsefulTestCase() {
 
         error.get()?.let { throw it }
     }
+
+    fun testConcurrentAccessToPropertyDelegate() {
+        compileAndRunProgram(KtTestUtil.getTestDataPathBase() + "/reflection/concurrentAccessToPropertyDelegate")
+    }
+
+    fun testConcurrentAccessToPrivateFunction() {
+        compileAndRunProgram(KtTestUtil.getTestDataPathBase() + "/reflection/concurrentAccessToPrivateFunction")
+    }
+
+    private fun compileAndRunProgram(root: String) {
+        val lib = CompilerTestUtil.compileJvmLibrary(File("$root/test.kt"))
+
+        runJava(
+            "-ea",
+            "-classpath",
+            listOf(
+                ForTestCompileRuntime.runtimeJarForTests().absolutePath,
+                ForTestCompileRuntime.reflectJarForTests().absolutePath,
+                lib.absolutePath,
+            ).joinToString(File.pathSeparator),
+            "TestKt",
+        )
+    }
+
+    private fun runJava(vararg args: String) {
+        val javaHome = System.getProperty("java.home")
+        val javaExe = File(javaHome, "bin/java.exe").takeIf(File::exists)
+            ?: File(javaHome, "bin/java").takeIf(File::exists)
+            ?: error("Can't find 'java' executable in $javaHome")
+
+        val process = ProcessBuilder(javaExe.absolutePath, *args).start()
+        process.waitFor(1, TimeUnit.MINUTES)
+        val stderr = process.errorStream.reader().readText()
+        val stdout = process.inputStream.reader().readText()
+        val exitCode = process.exitValue()
+        assertEquals("Program exited with exit code $exitCode.\nStdout:\n$stdout\nStderr:\n$stderr", 0, exitCode)
+    }
 }

core/reflection.jvm/src/kotlin/reflect/jvm/internal/KFunctionImpl.kt

@@ -23,6 +23,7 @@ import java.lang.reflect.Constructor
 import java.lang.reflect.Member
 import java.lang.reflect.Method
 import java.lang.reflect.Modifier
+import kotlin.LazyThreadSafetyMode.PUBLICATION
 import kotlin.jvm.internal.CallableReference
 import kotlin.jvm.internal.FunctionBase
 import kotlin.reflect.KFunction
@@ -58,7 +59,7 @@ internal class KFunctionImpl private constructor(
 
     override val name: String get() = descriptor.name.asString()
 
-    override val caller: Caller<*> by ReflectProperties.lazy caller@{
+    override val caller: Caller<*> by lazy(PUBLICATION) caller@{
         val member: Member? = when (val jvmSignature = RuntimeTypeMapper.mapSignature(descriptor)) {
             is KotlinConstructor -> {
                 if (isAnnotationConstructor)
@@ -89,7 +90,7 @@ internal class KFunctionImpl private constructor(
         }.createInlineClassAwareCallerIfNeeded(descriptor)
     }
 
-    override val defaultCaller: Caller<*>? by ReflectProperties.lazy defaultCaller@{
+    override val defaultCaller: Caller<*>? by lazy(PUBLICATION) defaultCaller@{
         val jvmSignature = RuntimeTypeMapper.mapSignature(descriptor)
         val member: Member? = when (jvmSignature) {
             is KotlinFunction -> {

core/reflection.jvm/src/kotlin/reflect/jvm/internal/KProperty0Impl.kt

@@ -28,9 +28,9 @@ internal open class KProperty0Impl<out V> : KProperty0<V>, KPropertyImpl<V> {
         container, name, signature, boundReceiver
     )
 
-    private val _getter = ReflectProperties.lazy { Getter(this) }
+    private val _getter = lazy(PUBLICATION) { Getter(this) }
 
-    override val getter: Getter<V> get() = _getter()
+    override val getter: Getter<V> get() = _getter.value
 
     override fun get(): V = getter.call()
 
@@ -52,9 +52,9 @@ internal class KMutableProperty0Impl<V> : KProperty0Impl<V>, KMutableProperty0<V
         container, name, signature, boundReceiver
     )
 
-    private val _setter = ReflectProperties.lazy { Setter(this) }
+    private val _setter = lazy(PUBLICATION) { Setter(this) }
 
-    override val setter: Setter<V> get() = _setter()
+    override val setter: Setter<V> get() = _setter.value
 
     override fun set(value: V) = setter.call(value)
 

core/reflection.jvm/src/kotlin/reflect/jvm/internal/KProperty1Impl.kt

@@ -28,9 +28,9 @@ internal open class KProperty1Impl<T, out V> : KProperty1<T, V>, KPropertyImpl<V
 
     constructor(container: KDeclarationContainerImpl, descriptor: PropertyDescriptor) : super(container, descriptor)
 
-    private val _getter = ReflectProperties.lazy { Getter(this) }
+    private val _getter = lazy(PUBLICATION) { Getter(this) }
 
-    override val getter: Getter<T, V> get() = _getter()
+    override val getter: Getter<T, V> get() = _getter.value
 
     override fun get(receiver: T): V = getter.call(receiver)
 
@@ -52,9 +52,9 @@ internal class KMutableProperty1Impl<T, V> : KProperty1Impl<T, V>, KMutablePrope
 
     constructor(container: KDeclarationContainerImpl, descriptor: PropertyDescriptor) : super(container, descriptor)
 
-    private val _setter = ReflectProperties.lazy { Setter(this) }
+    private val _setter = lazy(PUBLICATION) { Setter(this) }
 
-    override val setter: Setter<T, V> get() = _setter()
+    override val setter: Setter<T, V> get() = _setter.value
 
     override fun set(receiver: T, value: V) = setter.call(receiver, value)
 

core/reflection.jvm/src/kotlin/reflect/jvm/internal/KProperty2Impl.kt

@@ -29,9 +29,9 @@ internal open class KProperty2Impl<D, E, out V> : KProperty2<D, E, V>, KProperty
 
     constructor(container: KDeclarationContainerImpl, descriptor: PropertyDescriptor) : super(container, descriptor)
 
-    private val _getter = ReflectProperties.lazy { Getter(this) }
+    private val _getter = lazy(PUBLICATION) { Getter(this) }
 
-    override val getter: Getter<D, E, V> get() = _getter()
+    override val getter: Getter<D, E, V> get() = _getter.value
 
     override fun get(receiver1: D, receiver2: E): V = getter.call(receiver1, receiver2)
 
@@ -51,9 +51,9 @@ internal class KMutableProperty2Impl<D, E, V> : KProperty2Impl<D, E, V>, KMutabl
 
     constructor(container: KDeclarationContainerImpl, descriptor: PropertyDescriptor) : super(container, descriptor)
 
-    private val _setter = ReflectProperties.lazy { Setter(this) }
+    private val _setter = lazy(PUBLICATION) { Setter(this) }
 
-    override val setter: Setter<D, E, V> get() = _setter()
+    override val setter: Setter<D, E, V> get() = _setter.value
 
     override fun set(receiver1: D, receiver2: E, value: V) = setter.call(receiver1, receiver2, value)
 

core/reflection.jvm/src/kotlin/reflect/jvm/internal/KPropertyImpl.kt

@@ -15,6 +15,7 @@ import org.jetbrains.kotlin.resolve.isUnderlyingPropertyOfInlineClass
 import org.jetbrains.kotlin.serialization.deserialization.descriptors.DeserializedPropertyDescriptor
 import org.jetbrains.kotlin.types.TypeUtils
 import java.lang.reflect.*
+import kotlin.LazyThreadSafetyMode.PUBLICATION
 import kotlin.jvm.internal.CallableReference
 import kotlin.reflect.KFunction
 import kotlin.reflect.KMutableProperty
@@ -48,7 +49,7 @@ internal abstract class KPropertyImpl<out V> private constructor(
 
     override val isBound: Boolean get() = rawBoundReceiver != CallableReference.NO_RECEIVER
 
-    private val _javaField: ReflectProperties.LazyVal<Field?> = ReflectProperties.lazy {
+    private val _javaField = lazy(PUBLICATION) {
         when (val jvmSignature = RuntimeTypeMapper.mapPropertySignature(descriptor)) {
             is KotlinProperty -> {
                 val descriptor = jvmSignature.descriptor
@@ -75,7 +76,7 @@ internal abstract class KPropertyImpl<out V> private constructor(
         }
     }
 
-    val javaField: Field? get() = _javaField()
+    val javaField: Field? get() = _javaField.value
 
     protected fun computeDelegateSource(): Member? {
         if (!descriptor.isDelegated) return null
@@ -175,7 +176,7 @@ internal abstract class KPropertyImpl<out V> private constructor(
             property.descriptor.getter ?: DescriptorFactory.createDefaultGetter(property.descriptor, Annotations.EMPTY)
         }
 
-        override val caller: Caller<*> by ReflectProperties.lazy {
+        override val caller: Caller<*> by lazy(PUBLICATION) {
             computeCallerForAccessor(isGetter = true)
         }
 
@@ -196,7 +197,7 @@ internal abstract class KPropertyImpl<out V> private constructor(
             property.descriptor.setter ?: DescriptorFactory.createDefaultSetter(property.descriptor, Annotations.EMPTY, Annotations.EMPTY)
         }
 
-        override val caller: Caller<*> by ReflectProperties.lazy {
+        override val caller: Caller<*> by lazy(PUBLICATION) {
             computeCallerForAccessor(isGetter = false)
         }