From e32aa7815246b17fb604fdd8bae9f22cb4c237d3 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Wed, 22 Jun 2022 10:26:26 +0200 Subject: [PATCH 1/8] Ensure that all coroutines throwables in core are serializable Fixes #3328 --- ...ListAllCoroutineThrowableSubclassesTest.kt | 62 +++++++++++++++++++ kotlinx-coroutines-core/common/src/Timeout.kt | 4 +- kotlinx-coroutines-core/jvm/src/Exceptions.kt | 2 +- .../jvm/src/flow/internal/FlowExceptions.kt | 2 +- .../JobCancellationExceptionSerializerTest.kt | 42 +++++++++++++ 5 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 integration/kotlinx-coroutines-guava/test/ListAllCoroutineThrowableSubclassesTest.kt create mode 100644 kotlinx-coroutines-core/jvm/test/JobCancellationExceptionSerializerTest.kt diff --git a/integration/kotlinx-coroutines-guava/test/ListAllCoroutineThrowableSubclassesTest.kt b/integration/kotlinx-coroutines-guava/test/ListAllCoroutineThrowableSubclassesTest.kt new file mode 100644 index 0000000000..204d5e8302 --- /dev/null +++ b/integration/kotlinx-coroutines-guava/test/ListAllCoroutineThrowableSubclassesTest.kt @@ -0,0 +1,62 @@ +/* + * Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. + */ + +package kotlinx.coroutines.guava + +import com.google.common.reflect.* +import kotlinx.coroutines.* +import org.junit.Test +import kotlin.test.* + +class ListAllCoroutineThrowableSubclassesTest : TestBase() { + + /* + * These are all known throwables in kotlinx.coroutines. + * If you have added one, this test will fail to make + * you ensure your exception type is java.io.Serializable. + * + * We do not have means to check it automatically, so checks are delegated to humans. + * Also, this test meant to be in kotlinx-coroutines-core, but properly scanning classpath + * requires guava which is toxic dependency that we'd like to avoid even in tests. + * + * See #3328 for serialization rationale. + */ + private val knownThrowables = setOf( + "kotlinx.coroutines.TimeoutCancellationException", + "kotlinx.coroutines.JobCancellationException", + "kotlinx.coroutines.internal.UndeliveredElementException", + "kotlinx.coroutines.CompletionHandlerException", + "kotlinx.coroutines.DiagnosticCoroutineContextException", + "kotlinx.coroutines.CoroutinesInternalError", + "kotlinx.coroutines.channels.ClosedSendChannelException", + "kotlinx.coroutines.channels.ClosedReceiveChannelException", + "kotlinx.coroutines.flow.internal.ChildCancelledException", + "kotlinx.coroutines.flow.internal.AbortFlowException", + + ) + + @Test + fun testThrowableSubclassesAreSerializable() { + var throwables = 0 + val classes = ClassPath.from(this.javaClass.classLoader) + .getTopLevelClassesRecursive("kotlinx.coroutines"); + classes.forEach { + try { + if (Throwable::class.java.isAssignableFrom(it.load())) { + // Skip classes from test sources + if (it.load().protectionDomain.codeSource.location.toString().contains("/test/")) { + return@forEach + } + ++throwables + // println(""""$it",""") + assertTrue(knownThrowables.contains(it.toString())) + } + } catch (e: Throwable) { + // Ignore unloadable classes + } + } + + assertEquals(knownThrowables.size, throwables) + } +} diff --git a/kotlinx-coroutines-core/common/src/Timeout.kt b/kotlinx-coroutines-core/common/src/Timeout.kt index 46ab4ae8c8..6a6829df7a 100644 --- a/kotlinx-coroutines-core/common/src/Timeout.kt +++ b/kotlinx-coroutines-core/common/src/Timeout.kt @@ -163,7 +163,7 @@ private class TimeoutCoroutine( */ public class TimeoutCancellationException internal constructor( message: String, - @JvmField internal val coroutine: Job? + @JvmField @Transient internal val coroutine: Job? ) : CancellationException(message), CopyableThrowable { /** * Creates a timeout exception with the given message. @@ -173,7 +173,7 @@ public class TimeoutCancellationException internal constructor( internal constructor(message: String) : this(message, null) // message is never null in fact - override fun createCopy(): TimeoutCancellationException? = + override fun createCopy(): TimeoutCancellationException = TimeoutCancellationException(message ?: "", coroutine).also { it.initCause(this) } } diff --git a/kotlinx-coroutines-core/jvm/src/Exceptions.kt b/kotlinx-coroutines-core/jvm/src/Exceptions.kt index 007a0c98fa..48b4788cc5 100644 --- a/kotlinx-coroutines-core/jvm/src/Exceptions.kt +++ b/kotlinx-coroutines-core/jvm/src/Exceptions.kt @@ -29,7 +29,7 @@ public actual fun CancellationException(message: String?, cause: Throwable?) : C internal actual class JobCancellationException public actual constructor( message: String, cause: Throwable?, - @JvmField internal actual val job: Job + @JvmField @Transient internal actual val job: Job ) : CancellationException(message), CopyableThrowable { init { diff --git a/kotlinx-coroutines-core/jvm/src/flow/internal/FlowExceptions.kt b/kotlinx-coroutines-core/jvm/src/flow/internal/FlowExceptions.kt index d178060ddd..cfe5b69958 100644 --- a/kotlinx-coroutines-core/jvm/src/flow/internal/FlowExceptions.kt +++ b/kotlinx-coroutines-core/jvm/src/flow/internal/FlowExceptions.kt @@ -8,7 +8,7 @@ import kotlinx.coroutines.* import kotlinx.coroutines.flow.* internal actual class AbortFlowException actual constructor( - actual val owner: FlowCollector<*> + @JvmField @Transient actual val owner: FlowCollector<*> ) : CancellationException("Flow was aborted, no more elements needed") { override fun fillInStackTrace(): Throwable { diff --git a/kotlinx-coroutines-core/jvm/test/JobCancellationExceptionSerializerTest.kt b/kotlinx-coroutines-core/jvm/test/JobCancellationExceptionSerializerTest.kt new file mode 100644 index 0000000000..c909f27b64 --- /dev/null +++ b/kotlinx-coroutines-core/jvm/test/JobCancellationExceptionSerializerTest.kt @@ -0,0 +1,42 @@ +/* + * Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. + */ + +package kotlinx.coroutines + +import org.junit.* +import java.io.* + + +@Suppress("BlockingMethodInNonBlockingContext") +class JobCancellationExceptionSerializerTest : TestBase() { + + @Test + fun testSerialization() = runTest { + try { + coroutineScope { + expect(1) + + launch { + expect(2) + try { + hang {} + } catch (e: CancellationException) { + throw RuntimeException("RE2", e) + } + } + + launch { + expect(3) + throw RuntimeException("RE1") + } + } + } catch (e: Throwable) { + // Should not fail + ObjectOutputStream(ByteArrayOutputStream()).use { + it.writeObject(e) + } + finish(4) + } + } +} From 188ab3dea99b3ccce429a97325ba59f32a989f8a Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Wed, 22 Jun 2022 11:26:22 +0200 Subject: [PATCH 2/8] Move ListAllCoroutineThrowableSubclassesTest to integration-testing and fix assertion --- integration-testing/build.gradle | 9 ++++++++- .../ListAllCoroutineThrowableSubclassesTest.kt | 13 +++---------- 2 files changed, 11 insertions(+), 11 deletions(-) rename {integration/kotlinx-coroutines-guava/test => integration-testing/src/test/kotlin}/ListAllCoroutineThrowableSubclassesTest.kt (77%) diff --git a/integration-testing/build.gradle b/integration-testing/build.gradle index 60b6cdcd07..e87367e180 100644 --- a/integration-testing/build.gradle +++ b/integration-testing/build.gradle @@ -23,6 +23,13 @@ dependencies { } sourceSets { + test { + kotlin + dependencies { + implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:$coroutines_version" + implementation 'com.google.guava:guava:31.1-jre' + } + } mavenTest { kotlin compileClasspath += sourceSets.test.runtimeClasspath @@ -89,5 +96,5 @@ compileTestKotlin { } check { - dependsOn([mavenTest, debugAgentTest, coreAgentTest, 'smokeTest:build']) + dependsOn([test, mavenTest, debugAgentTest, coreAgentTest, 'smokeTest:build']) } diff --git a/integration/kotlinx-coroutines-guava/test/ListAllCoroutineThrowableSubclassesTest.kt b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt similarity index 77% rename from integration/kotlinx-coroutines-guava/test/ListAllCoroutineThrowableSubclassesTest.kt rename to integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt index 204d5e8302..6ff7103605 100644 --- a/integration/kotlinx-coroutines-guava/test/ListAllCoroutineThrowableSubclassesTest.kt +++ b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt @@ -2,14 +2,14 @@ * Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. */ -package kotlinx.coroutines.guava +package kotlinx.coroutines import com.google.common.reflect.* import kotlinx.coroutines.* import org.junit.Test import kotlin.test.* -class ListAllCoroutineThrowableSubclassesTest : TestBase() { +class ListAllCoroutineThrowableSubclassesTest { /* * These are all known throwables in kotlinx.coroutines. @@ -17,8 +17,6 @@ class ListAllCoroutineThrowableSubclassesTest : TestBase() { * you ensure your exception type is java.io.Serializable. * * We do not have means to check it automatically, so checks are delegated to humans. - * Also, this test meant to be in kotlinx-coroutines-core, but properly scanning classpath - * requires guava which is toxic dependency that we'd like to avoid even in tests. * * See #3328 for serialization rationale. */ @@ -33,7 +31,6 @@ class ListAllCoroutineThrowableSubclassesTest : TestBase() { "kotlinx.coroutines.channels.ClosedReceiveChannelException", "kotlinx.coroutines.flow.internal.ChildCancelledException", "kotlinx.coroutines.flow.internal.AbortFlowException", - ) @Test @@ -44,15 +41,11 @@ class ListAllCoroutineThrowableSubclassesTest : TestBase() { classes.forEach { try { if (Throwable::class.java.isAssignableFrom(it.load())) { - // Skip classes from test sources - if (it.load().protectionDomain.codeSource.location.toString().contains("/test/")) { - return@forEach - } ++throwables // println(""""$it",""") assertTrue(knownThrowables.contains(it.toString())) } - } catch (e: Throwable) { + } catch (e: LinkageError) { // Ignore unloadable classes } } From 0f7bbfeb5e25011965414877ccbe4781d459b2be Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Wed, 22 Jun 2022 11:28:35 +0200 Subject: [PATCH 3/8] Get rid of LinkageError completely as it's no longer relevant --- .../ListAllCoroutineThrowableSubclassesTest.kt | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt index 6ff7103605..3836151e92 100644 --- a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt +++ b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt @@ -39,14 +39,10 @@ class ListAllCoroutineThrowableSubclassesTest { val classes = ClassPath.from(this.javaClass.classLoader) .getTopLevelClassesRecursive("kotlinx.coroutines"); classes.forEach { - try { - if (Throwable::class.java.isAssignableFrom(it.load())) { - ++throwables - // println(""""$it",""") - assertTrue(knownThrowables.contains(it.toString())) - } - } catch (e: LinkageError) { - // Ignore unloadable classes + if (Throwable::class.java.isAssignableFrom(it.load())) { + ++throwables + // println(""""$it",""") + assertTrue(knownThrowables.contains(it.toString())) } } From 7b264ace41d0960d82c71a5783583ad796b89571 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Wed, 22 Jun 2022 12:51:54 +0300 Subject: [PATCH 4/8] Update integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com> --- .../src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt index 3836151e92..f497daa4db 100644 --- a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt +++ b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt @@ -41,8 +41,7 @@ class ListAllCoroutineThrowableSubclassesTest { classes.forEach { if (Throwable::class.java.isAssignableFrom(it.load())) { ++throwables - // println(""""$it",""") - assertTrue(knownThrowables.contains(it.toString())) + assertTrue(knownThrowables.contains(it.toString()), "Unknown throwable $it") } } From 4ff0411ae338dae7839227e20b62b3a440f9f7ae Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Wed, 22 Jun 2022 12:52:00 +0300 Subject: [PATCH 5/8] Update integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com> --- .../test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt index f497daa4db..717bc2cc90 100644 --- a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt +++ b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt @@ -12,8 +12,8 @@ import kotlin.test.* class ListAllCoroutineThrowableSubclassesTest { /* - * These are all known throwables in kotlinx.coroutines. - * If you have added one, this test will fail to make + * These are all the known throwables in kotlinx.coroutines. + * If you add one, this test will fail to make * you ensure your exception type is java.io.Serializable. * * We do not have means to check it automatically, so checks are delegated to humans. From 2d61ee783800b092afe5fa832afa6945f96efadd Mon Sep 17 00:00:00 2001 From: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com> Date: Wed, 22 Jun 2022 13:22:34 +0300 Subject: [PATCH 6/8] Update integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt --- .../kotlin/ListAllCoroutineThrowableSubclassesTest.kt | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt index 717bc2cc90..7707458abd 100644 --- a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt +++ b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt @@ -35,16 +35,9 @@ class ListAllCoroutineThrowableSubclassesTest { @Test fun testThrowableSubclassesAreSerializable() { - var throwables = 0 val classes = ClassPath.from(this.javaClass.classLoader) .getTopLevelClassesRecursive("kotlinx.coroutines"); - classes.forEach { - if (Throwable::class.java.isAssignableFrom(it.load())) { - ++throwables - assertTrue(knownThrowables.contains(it.toString()), "Unknown throwable $it") - } - } - - assertEquals(knownThrowables.size, throwables) + val throwables = classes.filter { Throwable::class.java.isAssignableFrom(it.load()) }.map { it.toString() } + assertContentEquals(knownThrowables.sorted(), throwables.sorted()) } } From 45fe8b8e72f81cf1332ccda50aad09db24349bb3 Mon Sep 17 00:00:00 2001 From: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com> Date: Wed, 22 Jun 2022 13:23:50 +0300 Subject: [PATCH 7/8] Update ListAllCoroutineThrowableSubclassesTest.kt --- .../src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt index 7707458abd..fefcc00528 100644 --- a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt +++ b/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt @@ -38,6 +38,6 @@ class ListAllCoroutineThrowableSubclassesTest { val classes = ClassPath.from(this.javaClass.classLoader) .getTopLevelClassesRecursive("kotlinx.coroutines"); val throwables = classes.filter { Throwable::class.java.isAssignableFrom(it.load()) }.map { it.toString() } - assertContentEquals(knownThrowables.sorted(), throwables.sorted()) + assertEquals(knownThrowables.sorted(), throwables.sorted()) } } From da1ffd6ab275dac50e01de967f6fe289c1df5977 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Wed, 22 Jun 2022 12:58:23 +0200 Subject: [PATCH 8/8] Rename test to withGuavaTest to avoid classpath pollution --- integration-testing/build.gradle | 14 ++++++++++++-- .../ListAllCoroutineThrowableSubclassesTest.kt | 0 2 files changed, 12 insertions(+), 2 deletions(-) rename integration-testing/src/{test => withGuavaTest}/kotlin/ListAllCoroutineThrowableSubclassesTest.kt (100%) diff --git a/integration-testing/build.gradle b/integration-testing/build.gradle index e87367e180..c23d35fbe7 100644 --- a/integration-testing/build.gradle +++ b/integration-testing/build.gradle @@ -23,8 +23,11 @@ dependencies { } sourceSets { - test { + withGuavaTest { kotlin + compileClasspath += sourceSets.test.runtimeClasspath + runtimeClasspath += sourceSets.test.runtimeClasspath + dependencies { implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:$coroutines_version" implementation 'com.google.guava:guava:31.1-jre' @@ -67,6 +70,13 @@ compileDebugAgentTestKotlin { } } +task withGuavaTest(type: Test) { + environment "version", coroutines_version + def sourceSet = sourceSets.withGuavaTest + testClassesDirs = sourceSet.output.classesDirs + classpath = sourceSet.runtimeClasspath +} + task mavenTest(type: Test) { environment "version", coroutines_version def sourceSet = sourceSets.mavenTest @@ -96,5 +106,5 @@ compileTestKotlin { } check { - dependsOn([test, mavenTest, debugAgentTest, coreAgentTest, 'smokeTest:build']) + dependsOn([withGuavaTest, mavenTest, debugAgentTest, coreAgentTest, 'smokeTest:build']) } diff --git a/integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt b/integration-testing/src/withGuavaTest/kotlin/ListAllCoroutineThrowableSubclassesTest.kt similarity index 100% rename from integration-testing/src/test/kotlin/ListAllCoroutineThrowableSubclassesTest.kt rename to integration-testing/src/withGuavaTest/kotlin/ListAllCoroutineThrowableSubclassesTest.kt