From 8f8305744e495eed1d371235e96cb5378307a202 Mon Sep 17 00:00:00 2001 From: Dmitry Khalanskiy <52952525+dkhalanskyjb@users.noreply.github.com> Date: Thu, 19 Dec 2024 12:48:12 +0100 Subject: [PATCH] Do not initialize non-mocked Dispatchers.Main unnecessarily (#4301) Before this change, the following could happen on the JVM: * `kotlinx.coroutines.test` accesses `Dispatchers.Main` before `setMain` is called. * `Dispatchers.Main` attempts to initialize *some other* Main dispatcher in addition to the `kotlinx-coroutines-test` Main dispatcher, if there is one. * If the `kotlinx-coroutines-android` artifact is present + its R8 rules are used to minify the tests, it's illegal to attempt to fail to create a `Main` dispatcher. `SUPPORT_MISSING = false` ensures that attempting to create a Main dispatcher fails immediately, in the call frame that attempts the creation, not waiting for `Dispatchers.Main` to be meaningfully used. In total, `kotlinx-coroutines-test` + `kotlinx-coroutines-android` + R8 minification of tests leads to some patterns of `kotlinx-coroutines-test` usage to crash. It turns out, though, that the problem originally reported was simply because of incorrect `kotlinx-coroutines-test` usage. The error message got improved to guide the programmer in such scenarios better. Fixes #4297 --- .../common/src/internal/TestMainDispatcher.kt | 23 ++++++++------ .../jvm/src/internal/TestMainDispatcherJvm.kt | 31 +++++++++++++++++-- .../test/ordered/tests/FirstMockedMainTest.kt | 2 +- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/kotlinx-coroutines-test/common/src/internal/TestMainDispatcher.kt b/kotlinx-coroutines-test/common/src/internal/TestMainDispatcher.kt index b94e261f7b..f9ab265c8d 100644 --- a/kotlinx-coroutines-test/common/src/internal/TestMainDispatcher.kt +++ b/kotlinx-coroutines-test/common/src/internal/TestMainDispatcher.kt @@ -9,31 +9,36 @@ import kotlin.coroutines.* * The testable main dispatcher used by kotlinx-coroutines-test. * It is a [MainCoroutineDispatcher] that delegates all actions to a settable delegate. */ -internal class TestMainDispatcher(delegate: CoroutineDispatcher): +internal class TestMainDispatcher(createInnerMain: () -> CoroutineDispatcher): MainCoroutineDispatcher(), Delay { - private val mainDispatcher = delegate - private var delegate = NonConcurrentlyModifiable(mainDispatcher, "Dispatchers.Main") + internal constructor(delegate: CoroutineDispatcher): this({ delegate }) + + private val mainDispatcher by lazy(createInnerMain) + private var delegate = NonConcurrentlyModifiable(null, "Dispatchers.Main") + + private val dispatcher + get() = delegate.value ?: mainDispatcher private val delay - get() = delegate.value as? Delay ?: defaultDelay + get() = dispatcher as? Delay ?: defaultDelay override val immediate: MainCoroutineDispatcher - get() = (delegate.value as? MainCoroutineDispatcher)?.immediate ?: this + get() = (dispatcher as? MainCoroutineDispatcher)?.immediate ?: this - override fun dispatch(context: CoroutineContext, block: Runnable) = delegate.value.dispatch(context, block) + override fun dispatch(context: CoroutineContext, block: Runnable) = dispatcher.dispatch(context, block) - override fun isDispatchNeeded(context: CoroutineContext): Boolean = delegate.value.isDispatchNeeded(context) + override fun isDispatchNeeded(context: CoroutineContext): Boolean = dispatcher.isDispatchNeeded(context) - override fun dispatchYield(context: CoroutineContext, block: Runnable) = delegate.value.dispatchYield(context, block) + override fun dispatchYield(context: CoroutineContext, block: Runnable) = dispatcher.dispatchYield(context, block) fun setDispatcher(dispatcher: CoroutineDispatcher) { delegate.value = dispatcher } fun resetDispatcher() { - delegate.value = mainDispatcher + delegate.value = null } override fun scheduleResumeAfterDelay(timeMillis: Long, continuation: CancellableContinuation) = diff --git a/kotlinx-coroutines-test/jvm/src/internal/TestMainDispatcherJvm.kt b/kotlinx-coroutines-test/jvm/src/internal/TestMainDispatcherJvm.kt index 1b2d7533de..9626c7a87a 100644 --- a/kotlinx-coroutines-test/jvm/src/internal/TestMainDispatcherJvm.kt +++ b/kotlinx-coroutines-test/jvm/src/internal/TestMainDispatcherJvm.kt @@ -8,8 +8,23 @@ internal class TestMainDispatcherFactory : MainDispatcherFactory { override fun createDispatcher(allFactories: List): MainCoroutineDispatcher { val otherFactories = allFactories.filter { it !== this } val secondBestFactory = otherFactories.maxByOrNull { it.loadPriority } ?: MissingMainCoroutineDispatcherFactory - val dispatcher = secondBestFactory.tryCreateDispatcher(otherFactories) - return TestMainDispatcher(dispatcher) + /* Do not immediately create the alternative dispatcher, as with `SUPPORT_MISSING` set to `false`, + it will throw an exception. Instead, create it lazily. */ + return TestMainDispatcher({ + val dispatcher = try { + secondBestFactory.tryCreateDispatcher(otherFactories) + } catch (e: Throwable) { + reportMissingMainCoroutineDispatcher(e) + } + if (dispatcher.isMissing()) { + reportMissingMainCoroutineDispatcher(runCatching { + // attempt to dispatch something to the missing dispatcher to trigger the exception. + dispatcher.dispatch(dispatcher, Runnable { }) + }.exceptionOrNull()) // can not be null, but it does not matter. + } else { + dispatcher + } + }) } /** @@ -24,4 +39,14 @@ internal actual fun Dispatchers.getTestMainDispatcher(): TestMainDispatcher { val mainDispatcher = Main require(mainDispatcher is TestMainDispatcher) { "TestMainDispatcher is not set as main dispatcher, have $mainDispatcher instead." } return mainDispatcher -} \ No newline at end of file +} + +private fun reportMissingMainCoroutineDispatcher(e: Throwable? = null): Nothing { + throw IllegalStateException( + "Dispatchers.Main was accessed when the platform dispatcher was absent " + + "and the test dispatcher was unset. Please make sure that Dispatchers.setMain() is called " + + "before accessing Dispatchers.Main and that Dispatchers.Main is not accessed after " + + "Dispatchers.resetMain().", + e + ) +} diff --git a/ui/kotlinx-coroutines-android/android-unit-tests/test/ordered/tests/FirstMockedMainTest.kt b/ui/kotlinx-coroutines-android/android-unit-tests/test/ordered/tests/FirstMockedMainTest.kt index 30f1796ef6..f56e4c4bdd 100644 --- a/ui/kotlinx-coroutines-android/android-unit-tests/test/ordered/tests/FirstMockedMainTest.kt +++ b/ui/kotlinx-coroutines-android/android-unit-tests/test/ordered/tests/FirstMockedMainTest.kt @@ -35,7 +35,7 @@ open class FirstMockedMainTest : TestBase() { component.launchSomething() throw component.caughtException } catch (e: IllegalStateException) { - assertTrue(e.message!!.contains("Dispatchers.setMain from kotlinx-coroutines-test")) + assertTrue(e.message!!.contains("Dispatchers.setMain")) } } }