Ensure Dispatchers.Main != Dispatchers.Main.immediate on Android

[dkhalanskyjb]

Fixes #3545
Fixes #3963

[qwwdfsad]

This change changes the behaviour of the following code:

@Test
    fun foo() = runBlocking<Unit> {
        withContext(Dispatchers.Main) {
            println("1")
            withContext(Dispatchers.Main.immediate) {
                println("2")
                launch(Dispatchers.Main) {
                    println("4")
                }
                withContext(Dispatchers.Main) {
                    println("3")
                }
            }
        }
    }

It seems like this is expected (at least from the issue description), and I would rather expect the current (after your change) behaviour, but still pointing it out just in case

[dkhalanskyjb]

Oh. Didn't realize this. Then, we'll have to weigh the pros and cons.

I checked all the other Dispatchers.Main.immediate implementations, and they all use object identity for equality checks, so this change is also needed simply for consistency.

Also, before this change, the behavior of

            withContext(Dispatchers.Main.immediate) {
                println("2")
                launch(Dispatchers.Main) {
                    println("4")
                }
                withContext(Dispatchers.Main) {
                    println("3")
                }
            }

was 2, 4, 3 if it was not called from the main thread, but 2, 3, 4 it it was. Now, it's consistently 2, 4, 3. I think it's more intuitive that the calling thread doesn't affect the ordering.

Also, yes, I, too, would expect the proposed behavior:

fun updateUi() = scope.launch(Dispatchers.Main) { }

suspend fun readUiElement(): Int = withContext(Dispatchers.Main) { 1 }

fun update() = viewModelScope.launch {
  updateUi()
  cache = readUiElement()
}

I think it's reasonable to expect that readUiElement detects the results of updateUi. Currently, for this to work reliably, I think a yield would need to be inserted between the two lines.

... That said, of course, this will subtly break someone's code that relied on the existing implicit ordering.

I'd say the pros outweigh the cons. Most people would be better served by having the execution order roughly the same as the order of function invocations.

[qwwdfsad]

Then it's worth doing the following:

• Fix this behaviour with tests explicitly (in all integrations)
• Maybe document it around immediate property
• Rephrase the issue (or create a new one) in a bit more clear manner so we'll reference it in the changelog properly

It's okay to do it both in and out of the scope of this issue. WDYT?

[dkhalanskyjb]

From the latest build failure:

e: integration-testing/smokeTest/src/commonMain/kotlin/Sample.kt:4:5 Cannot access class 'kotlin.coroutines.CoroutineContext'. Check your module classpath for missing or conflicting dependencies
e: integration-testing/smokeTest/src/commonMain/kotlin/Sample.kt:6:9 Unresolved reference: println
e: integration-testing/smokeTest/src/commonMain/kotlin/Sample.kt:8:5 Unresolved reference: println
e: org.jetbrains.kotlin.util.KotlinFrontEndException: Exception while analyzing expression in (3,25) in integration-testing/smokeTest/src/commonMain/kotlin/Sample.kt
Attachments:
causeThrowable
java.lang.AssertionError: Built-in class kotlin.Any is not found

and

Execution failed for task ':kotlinx-coroutines-swing:koverVerify'. kotlinx.kover.gradle.plugin.commons.KoverVerificationException: Rule violated: lines covered percentage is 53.571400, but expected minimum is 70

Oh, mighty computing gods, please forgive me for all my sins! I repent twice, thrice, and however many times is needed. I knew I shouldn't have written that compile-time VM with the C++ templates at the university! I implore you, please, spare me and allow me to proceed with this humble pull request. Amen.

[qwwdfsad]

Behold, thou shalt not ignore the tests, lest thy build shell not pass!

The first failure is penned by those who have sinned. Thou shalt not seek it -- the sorrow shalt thou find in Gradle

[qwwdfsad]

Nice one

[qwwdfsad]

I wonder why you've used xor instead of IDEA-generated polynomial hash (31 * ...).
No strong opinion here though

[dkhalanskyjb]

These are some tough questions you're asking. Let's look at whether collisions are possible.

1231 + (1 - invokeImmediately1) * 6 + 31 * handler1 == 1231 + (1 - invokeImmediately2) * 6 + 31 * handler2 (mod 2^32)

If invokeImmediately1 == invokeImmediately2, then handler1 == handler2 (mod 2^32), by the virtue of all operations here being bijective modulo 2^64. If they are different, we get

1237 + 31 * handler1 == 1231 + 31 * handler2 (mod 2^32)

or

31 * (handler2 - handler1) == 6 (mod 2^32)

This is possible if handler2 - handler1 == 1_939_662_650.

With handler1, handler2 in [0; 2^32), we get around 2^32 - 1_939_662_650 == 2_355_304_646 unordered pairs where this can happen, or 2.5 * 10^(-10) chance if they are distributed uniformly.

Let's look at

handler1 xor (1231 + (1 - invokeImmediately1) * 6) == handler2 xor (1231 + (1 - invokeImmediately2) * 6)

No modulo arithmetic this time. Again, handler1 == handler2 if invokeImmediately1 == invokeImmediately2. Otherwise,

handler1 xor 1237 == handler2 xor 1231

Equivalently,

handler1 xor handler2 == 1237 xor 1231 == 0b11010

So, the collision will happen exactly if handler1 and handler2 differ in exactly the bits 2, 4, and 5. For every number in [0; 2^32), there's exactly one pair for it that causes a collision, so there are about 2^31 == 2_147_483_648 unordered pairs where this can happen, or 2.3 * 10^(-10) chance if handler1 and handler2 are distributed uniformly.

Intuitively, a number like 2_000_000_000 can collide with two numbers in the polynomial hash but with only one in the xor hash, and every number clashes with at least one other number in both cases.

So, the xor hash has insignificantly better theoretical properties for the case when the colliding objects are both HandlerDispatcher instances.

What led me to using it, however, was that it was a bit shorter to write while still keeping the bijective properties when any parameter is fixed.

[dkhalanskyjb]

Worth noting that choosing some other constant for the polynomial hash could lead us to a much better situation like only a handful of collisions being possible at all. I don't think it's worth it to search for the required constants, though, as no one will ever be sad that their HashSet<MainCoroutineDispatcher> on Android has a 2 * 10^(-10) collision chance for every pair.