Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't call Dispatchers.Main getter during UnconfinedTestDispatcher construction #4297

Closed
azabost opened this issue Dec 12, 2024 · 5 comments
Closed

Comments

@azabost
Copy link

azabost commented Dec 12, 2024

I'm opening this issue as a result of my short conversation with @dkhalanskyjb on Kotlin Slack: https://kotlinlang.slack.com/archives/C1CFAFJSK/p1734007891082299

What do we have now?

In certain situations, creating an instance of UnconfinedTestDispatcher by calling UnconfinedTestDispatcher() just before setting it as the main dispatcher via Dispatchers.setMain, e.g. in a JUnit rule, may fail.

(The same applies to StandardTestDispatcher)

The execution path of creating the dispatcher was like this:

  1. UnconfinedTestDispatcher
    scheduler ?: TestMainDispatcher.currentTestScheduler ?: TestCoroutineScheduler(), name)
  2. TestMainDispatcher.currentTestScheduler
  3. TestMainDispatcher.currentTestDispatcher
    get() = (Dispatchers.Main as? TestMainDispatcher)?.delegate?.value as? TestDispatcher
  4. Dispatchers.Main (getter invoked before Dispatchers.setMain!)
    public actual val Main: MainCoroutineDispatcher get() = MainDispatcherLoader.dispatcher
  5. MainDispatcherLoader.loadMainDispatcher
  6. MainDispatchersKt.tryCreateDispatcher
  7. kotlinx.coroutines.android.AndroidDispatcherFactory.createDispatcher
  8. Looper.getMainLooper() ⚡ throws
Stacktrace
kotlinx.coroutines.CoroutinesInternalError: Fatal exception in coroutines machinery for DispatchedContinuation[Dispatchers.IO, Continuation at kotlinx.coroutines.flow.internal.ChannelFlow$collectToFun$1.invokeSuspend(ChannelFlow.kt:56)@6c0c966f]. Please read KDoc to 'handleFatalException' method and report this incident to maintainers
	at app//kotlinx.coroutines.DispatchedTask.handleFatalException$kotlinx_coroutines_core(DispatchedTask.kt:142)
	at app//kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:113)
	at app//kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:111)
	at app//kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:99)
	at app//kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:584)
	at app//kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:811)
	at app//kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:715)
	at app//kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:702)
Caused by: java.lang.IllegalStateException: Module with the Main dispatcher had failed to initialize. For tests Dispatchers.setMain from kotlinx-coroutines-test module can be used
	at kotlinx.coroutines.internal.MissingMainCoroutineDispatcher.missing(MainDispatchers.kt:111)
	at kotlinx.coroutines.internal.MissingMainCoroutineDispatcher.isDispatchNeeded(MainDispatchers.kt:92)
	at kotlinx.coroutines.test.internal.TestMainDispatcher.isDispatchNeeded(TestMainDispatcher.kt:27)
	at kotlinx.coroutines.DispatchedTaskKt.dispatch(DispatchedTask.kt:156)
	at kotlinx.coroutines.CancellableContinuationImpl.dispatchResume(CancellableContinuationImpl.kt:466)
	at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl(CancellableContinuationImpl.kt:500)
	at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl$default(CancellableContinuationImpl.kt:489)
	at kotlinx.coroutines.CancellableContinuationImpl.resumeWith(CancellableContinuationImpl.kt:364)
	at kotlinx.coroutines.channels.BufferedChannel$BufferedChannelIterator.tryResumeHasNextOnClosedChannel(BufferedChannel.kt:1737)
	at kotlinx.coroutines.channels.BufferedChannel.resumeWaiterOnClosedChannel(BufferedChannel.kt:2204)
	at kotlinx.coroutines.channels.BufferedChannel.resumeReceiverOnClosedChannel(BufferedChannel.kt:2191)
	at kotlinx.coroutines.channels.BufferedChannel.cancelSuspendedReceiveRequests(BufferedChannel.kt:2184)
	at kotlinx.coroutines.channels.BufferedChannel.completeClose(BufferedChannel.kt:1961)
	at kotlinx.coroutines.channels.BufferedChannel.isClosed(BufferedChannel.kt:2240)
	at kotlinx.coroutines.channels.BufferedChannel.isClosedForSend0(BufferedChannel.kt:2215)
	at kotlinx.coroutines.channels.BufferedChannel.isClosedForSend(BufferedChannel.kt:2212)
	at kotlinx.coroutines.channels.BufferedChannel.completeCloseOrCancel(BufferedChannel.kt:1933)
	at kotlinx.coroutines.channels.BufferedChannel.closeOrCancelImpl(BufferedChannel.kt:1826)
	at kotlinx.coroutines.channels.BufferedChannel.close(BufferedChannel.kt:1785)
	at kotlinx.coroutines.channels.SendChannel$DefaultImpls.close$default(Channel.kt:95)
	at kotlinx.coroutines.channels.ProducerCoroutine.onCompleted(Produce.kt:140)
	at kotlinx.coroutines.channels.ProducerCoroutine.onCompleted(Produce.kt:133)
	at kotlinx.coroutines.AbstractCoroutine.onCompletionInternal(AbstractCoroutine.kt:90)
	at kotlinx.coroutines.JobSupport.tryFinalizeSimpleState(JobSupport.kt:293)
	at kotlinx.coroutines.JobSupport.tryMakeCompleting(JobSupport.kt:867)
	at kotlinx.coroutines.JobSupport.makeCompletingOnce$kotlinx_coroutines_core(JobSupport.kt:839)
	at kotlinx.coroutines.AbstractCoroutine.resumeWith(AbstractCoroutine.kt:97)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:46)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:104)
	... 6 more
Caused by: java.lang.IllegalStateException: The main looper is not available
	at kotlinx.coroutines.android.AndroidDispatcherFactory.createDispatcher(HandlerDispatcher.kt:51)
	at kotlinx.coroutines.internal.MainDispatchersKt.tryCreateDispatcher(MainDispatchers.kt:53)
	at kotlinx.coroutines.test.internal.TestMainDispatcherFactory.createDispatcher(TestMainDispatcherJvm.kt:11)
	at kotlinx.coroutines.internal.MainDispatchersKt.tryCreateDispatcher(MainDispatchers.kt:53)
	at kotlinx.coroutines.internal.MainDispatcherLoader.loadMainDispatcher(MainDispatchers.kt:34)
	at kotlinx.coroutines.internal.MainDispatcherLoader.<clinit>(MainDispatchers.kt:18)
	at kotlinx.coroutines.Dispatchers.getMain(Dispatchers.kt:20)
	at kotlinx.coroutines.test.internal.TestMainDispatcher$Companion.getCurrentTestDispatcher$kotlinx_coroutines_test(TestMainDispatcher.kt:47)
	at kotlinx.coroutines.test.internal.TestMainDispatcher$Companion.getCurrentTestScheduler$kotlinx_coroutines_test(TestMainDispatcher.kt:50)
	at kotlinx.coroutines.test.TestCoroutineDispatchersKt.UnconfinedTestDispatcher(TestCoroutineDispatchers.kt:83)
	at kotlinx.coroutines.test.TestCoroutineDispatchersKt.UnconfinedTestDispatcher$default(TestCoroutineDispatchers.kt:79)
	at com.example.MainDispatcherRule.<init>(MainDispatcherRule.kt:14)
	at com.example.Test.<init>(Test.kt:51)
	(...)

It prevented me from setting the UnconfinedTestDispatcher as Main because it wasn't possible to construct it in the first place. The error also wasn't helpful because it told me to call setMain which happened to be the exact thing I was trying to accomplish.

To be perfectly honest, I'm not sure how exactly to reproduce these circumstances. The project where I stumbled upon this problem was large and had a complicated setup. I wouldn't rule out the possibility of a user error or a misconfiguration of some sort, such as a version conflict in dependencies or a test classpath pollution (e.g. having ...-android in the test classpath). Nevertheless, it would be great if this problem could be avoided somehow.

What should be instead?

If possible, the code necessary for substituting the main dispatcher (such as the UnconfinedTestDispatcher() or StandardTestDispatcher() function) should not call Dispatchers.Main (getter) because it leads to strange situations like the one I described where the user is told to call setMain even though that's exactly what the user is trying to accomplish.

Why?

The upsides of your proposal.

  • Who would benefit from this and how? - People who accidentally ended up with coroutines-android in unit tests, I suppose
    • They wouldn't be told to do exactly the thing they are trying to do when they get the exception

Why not?

The downsides of your proposal that you already see.

  • Is this a breaking change? - Probably not
  • Are there use cases that are better solved by what we have now? - I don't know
  • Does some code become less clear after this change? - Good question
  • It was the only project where I found this problem and it makes me believe it's a user error after all.
@azabost azabost added the design label Dec 12, 2024
@azabost
Copy link
Author

azabost commented Dec 12, 2024

I'm seriously considering withdrawing this issue because I no longer feel like I know what I'm talking about. Initially, I thought I found some workarounds that made me believe I knew more or less where the problem came from, but it later turned out that whenever I saw an improvement in one test, I was re-discovering the same kind of failure (often as a suppressed exception) somewhere else. I'm still trying to understand what is happening in this project where I found this problem.

I will wait for some feedback on this but feel free to close.

@dkhalanskyjb
Copy link
Collaborator

Hi! As I mentioned in Kotlinlang, this issue can be worked around on our side: we really are doing unnecessary work which theoretically can fail, and the fix should be fairly straightforward. Even if it turns out not to be a problem in practice and it's your project that's doing something strange, we can still eliminate this problem entirely as a class.

@dkhalanskyjb
Copy link
Collaborator

I've looked into this, and here are the specific conditions for this to occur:

  1. kotlinx-coroutines-android has to be present in the classpath.
  2. R8 1.6.0 and later must be used to minify the tests! In our R8 config, we have essentially a flag: "If the Main module failed to initialize, fail immediately."

To my knowledge, tests are usually not minified, which can explain why we haven't encountered this before.

dkhalanskyjb added a commit that referenced this issue Dec 16, 2024
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.

Fixes #4297
dkhalanskyjb added a commit that referenced this issue Dec 16, 2024
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.

Fixes #4297
dkhalanskyjb added a commit that referenced this issue Dec 16, 2024
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.

Fixes #4297
azabost added a commit to azabost/coroutines-4297-repro that referenced this issue Dec 16, 2024
@azabost
Copy link
Author

azabost commented Dec 16, 2024

@dkhalanskyjb

R8 1.6.0 and later must be used to minify the tests! In our R8 config, we have essentially a flag: "If the Main module failed to initialize, fail immediately."

To my knowledge, tests are usually not minified, which can explain why we haven't encountered this before.

In the project where I found this problem, R8 was disabled 🤷‍♂️

However, I would like to add that I finally identified the real root cause of many problems I encountered in that project. I realized the production code under test was written in a test-unfriendly manner. For example, there were dangling coroutines launched on non-test dispatchers, throwing exceptions in the background and causing failures of the subsequent tests using runTest with UncaughtExceptionsBeforeTest. I reproduced a similar problem in this demo repo: https://github.com/azabost/coroutines-4297-repro

That being said, I'm unsure if my original request Don't call Dispatchers.Main getter during UnconfinedTestDispatcher construction makes any sense as the problem I encountered was merely a symptom of a coroutines misuse in that project. I'm leaning towards closing this issue unless you want to keep it open despite all I said above.

@dkhalanskyjb
Copy link
Collaborator

Yes, after reading your stacktrace more carefully, I do see that it's because some coroutines run in Dispatchers.Main while the Main dispatcher is not replaced with the test-only one, R8 is not at fault here. I'll think of ways to produce a better error message in this scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants