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

Dispatchers.setMain does not implement a correct Dispatchers.Main.immediate #3298

Open
dkhalanskyjb opened this issue May 20, 2022 · 1 comment

Comments

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented May 20, 2022

https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-main-coroutine-dispatcher/immediate.html states that, if Dispatchers.Main.immediate is supported, it behaves like Dispatchers.Unconfined when already in the correct execution context; otherwise, it should throw UnsupportedOperationException. However, this is not the case for Dispatchers.setMain, which will just use the dispatcher itself for Dispatchers.Main.immediate, without preventing stack overflows or eager execution of coroutines.

This causes real problems in the test code. The main example is viewModelScope, a CoroutineScope that corresponds to a ViewScope, Android-specific storage of data for use in UI. Because of being UI-related, viewModelScope uses the UI thread by default, and to avoid unnecessary dispatches when working with a ViewModel from the main thread, it uses Dispatchers.Main.immediate.

Code that tests functions using viewModelScope is typically structured in a way that expects the dispatches to happen eagerly—corresponding to real-world behavior in the typical case—which necessitates the use of UnconfinedTestDispatcher in Dispatchers.setMain for such tests. However, this will lead to incorrect dispatching behavior for just Dispatchers.Main, which is supposed to behave like a StandardTestDispatcher.

The most straightforward idea is two-part:

1. Make StandardTestDispatcher a MainCoroutineDispatcher, confined to the thread and exposing a proper immediate.

Problems:

  • What is the test thread? If we just check the thread at the time of creation of the test dispatcher, can this can cause problems with multithreaded test runners?
  • Calls to advance*/runCurrent from other threads will hang if we implement this naively. Maybe a better idea is not to confine the execution to one thread, but to consider the thread doing the innermost advance*/runCurrent to be the "main" thread.

2. Throw UnsupportedOperationException for Dispatchers.Main.immediate for non-MainCoroutineDispatcher arguments to Dispatchers.setMain.

This is a breaking change.

  • We could lessen the issue by also making UnconfinedTestDispatcher a MainCoroutineDispatcher, but that would promote its incorrect use.
  • Maybe, instead of breaking Dispatchers.setMain, we should deprecate it in favor of something else. For example, if we end up implementing Test module shall provide ability to replace Dispatchers.IO and Dispatchers.Default #982, we could just provide something like Dispatchers.mock(main, default) that would also fix the issue with Main.immediate.
@joaoeudes7
Copy link

I'm using the temp solution: Dispatchers.setMain(Dispatchers.Unconfined), but waiting for the official resolution from the library

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