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

Android: Main.immediate causes a dispatch using Unconfined inside #1650

Closed
vganin opened this issue Nov 7, 2019 · 6 comments
Closed

Android: Main.immediate causes a dispatch using Unconfined inside #1650

vganin opened this issue Nov 7, 2019 · 6 comments
Labels
docs KDoc and API reference question

Comments

@vganin
Copy link

vganin commented Nov 7, 2019

Dispatchers.Main.immediate forces child coroutine with Dispatchers.Unconfined interceptor to dispatch.

Example code

GlobalScope.launch(Dispatchers.Main.immediate) {
    GlobalScope.launch(Dispatchers.Unconfined) {
        println("$coroutineContext: before")
    }
    println("$coroutineContext: after")
}

This code prints

[CoroutineId(1), "coroutine#1":StandaloneCoroutine{Active}@229841e, Main [immediate]]: after
[CoroutineId(2), "coroutine#2":StandaloneCoroutine{Active}@f2294ff, Unconfined]: before

But expected would be backwards.

I can force not to dispatch if I use start = CoroutineStart.UNDISPATCHED. Then I get the expected result

[CoroutineId(4), "coroutine#4":StandaloneCoroutine{Active}@229841e, Unconfined]: before
[CoroutineId(3), "coroutine#3":StandaloneCoroutine{Active}@f2294ff, Main [immediate]]: after

But shouldn't GlobalScope.launch(Dispatchers.Unconfined) and GlobalScope.launch(start = CoroutineStart.UNDISPATCHED, context = Dispatchers.Unconfined) be interchangable?

@vganin
Copy link
Author

vganin commented Nov 7, 2019

By the way using Dispatchers.Main yield correct result

[CoroutineId(29), "coroutine#29":StandaloneCoroutine{Active}@c6ed5c2, Unconfined]: before
[CoroutineId(25), "coroutine#25":StandaloneCoroutine{Active}@65e4fd3, Main]: after

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Nov 8, 2019

This is a protection to avoid spurious stack overflows when you have (usually implicit) nested unconfined coroutines. Otherwise, the simplest ping-pong with a channel and two unconfined coroutines would fail with StackOverflowError.

This behaviour reflected both in Unconfined and immediate dispatchers documentation:

Immediate dispatcher is safe from stack overflows and in case of nested invocations forms event-loop similar to [Dispatchers.Unconfined].
The event loop is an advanced topic and its implications can be found in [Dispatchers.Unconfined] documentation.

Nested coroutines launched in this dispatcher form an event-loop to avoid stack overflows.
... long description of what event loop is and its implications ...

@vganin
Copy link
Author

vganin commented Nov 8, 2019

Well I didn't think that immediate and Unconfined share the same loop in the same thread but it perfectly makes sense. Otherwise it would not work.

I think the documentation to CoroutineStart.UNDISPATCHED is a bit misleading:

Immediately executes the coroutine until its first suspension point in the current thread as if the coroutine was started using [Dispatchers.Unconfined].

The part about "as if the coroutine was started using [Dispatchers.Unconfined]" seems false when we are dealing with nested unconfined coroutines.

@qwwdfsad qwwdfsad added the docs KDoc and API reference label Nov 8, 2019
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Nov 8, 2019

Good point, I will adjust our documentation accordingly

@vganin
Copy link
Author

vganin commented Nov 21, 2019

Btw is there a way to force a non-dispatch in cases where it is known that no stack overflow can occur (I mean within reasonable limits). For example, if I have two coroutines but the data flows only one way. In that case dispatch is superfluous (and can create data races). Look at the following example:

GlobalScope.launch(Dispatchers.Main.immediate) {
    val c = Channel<Int>()
    launch {
        c.consumeAsFlow()
                .collect {
                    println(it)
                }
    }
    launch {
        c.send(1)
        c.send(2)
    }
}

I would like to force such statement execution order:

c.send(1)
println(1)
c.send(2)
println(2)

Instead I always get

c.send(1)
c.send(2)
println(1)
println(2)

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Nov 21, 2019

It is a mix of "immediate" interaction with channel unfairness (#111).
Two solutions here:

  1. Yielding after send to make sending to the channel fair
  2. Launching sender on main dispatcher and receiver on immediate one:
GlobalScope.launch(Dispatchers.Main) {
    val c = Channel<Int>()
    launch(Dispatchers.Main.immediate) {
        c.consumeAsFlow()
                .collect {
                    println(it)
                }
    }
    launch {
        c.send(1)
        c.send(2)
   }
}

No way to force this behaviour on the level of a dispatcher, because it's easy to make a mistake here e.g. during code evolution, not at the moment of writing, thus we don't endorse this.
In theory, a sophisticated user can write its own dispatcher around Dispatchers.Main that behaves
in a way you want, but we neither advocate it nor have an extensive test suite for such dispatchers (thus it may break in weird ways)

qwwdfsad added a commit that referenced this issue Nov 25, 2019
    * Clarifications of CoroutineExceptionHandler execution
    * Clarifications of MainCoroutineDispatcher.immediate
    * Outdated documentation (pointed out by Google AndoidX team) for isDispatchNeeded is rewritten
    *

Fixes #1650
Fixes #1651
Fixes #1634
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs KDoc and API reference question
Projects
None yet
Development

No branches or pull requests

2 participants