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

Several issues with the DefaultExecutor #4262

Open
dkhalanskyjb opened this issue Oct 29, 2024 · 0 comments
Open

Several issues with the DefaultExecutor #4262

dkhalanskyjb opened this issue Oct 29, 2024 · 0 comments

Comments

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Oct 29, 2024

What is DefaultExecutor?

DefaultExecutor is a strange thread that is available out-of-the-box with kotlinx-coroutines on the Kotlin/JVM and Kotlin/Native implementations (JS and Wasm are not important for this discussion) and is used for two purposes.

Scheduling

To process small fragments of code after a given delay: for example, this is the thread that calls dispatch after delay(1000) is done waiting.

Leaving no code behind

import kotlinx.coroutines.*

fun main() {
    val dispatcher = runBlocking {
        coroutineContext[CoroutineDispatcher.Key]
    }!!
    GlobalScope.launch(dispatcher) {
        println(Thread.currentThread())
    }
    Thread.sleep(1000)
}

prints

Thread[kotlinx.coroutines.DefaultExecutor @coroutine#2,5,main]

In some cases, coroutines can be started on a dispatcher that's no longer available, and occasionally (though not always), we use DefaultExecutor to process the code in place of that dispatcher. This functionality is not needed unless structured concurrency is broken, but it's still something we have to keep in mind.

What are the issues?

Liveness suffers

Dispatchers.Unconfined (and custom dispatchers) can execute the tasks in-place in the dispatch call.

This means that using delay in Dispatchers.Unconfined is a sure way to make the thread processing all the delays busy with arbitrary work:

import kotlinx.coroutines.*

fun main() {
    val start = kotlin.time.TimeSource.Monotonic.markNow()
    runBlocking(Dispatchers.Default) {
        launch(Dispatchers.Unconfined) {
            println("A (${start.elapsedNow()}) at ${Thread.currentThread()}. Sleeping for 100 milliseconds")
            delay(100)
            println("A (${start.elapsedNow()}) at ${Thread.currentThread()}. Beginning to work for 500 milliseconds")
            Thread.sleep(500)
            println("A (${start.elapsedNow()}) at ${Thread.currentThread()}. Finished work")
        }
        launch {
            println("B (${start.elapsedNow()}) at ${Thread.currentThread()}. Sleeping for 150 milliseconds")
            delay(150)
            println("B (${start.elapsedNow()}) at ${Thread.currentThread()}. Awoken")
        }
    }
}

prints

A (110.169892ms) at Thread[DefaultDispatcher-worker-1 @coroutine#2,5,main]. Sleeping for 100 milliseconds
B (132.386893ms) at Thread[DefaultDispatcher-worker-2 @coroutine#3,5,main]. Sleeping for 150 milliseconds
A (224.130426ms) at Thread[kotlinx.coroutines.DefaultExecutor @coroutine#2,5,main]. Beginning to work for 500 milliseconds
A (724.428710ms) at Thread[kotlinx.coroutines.DefaultExecutor @coroutine#2,5,main]. Finished work
B (725.101031ms) at Thread[DefaultDispatcher-worker-1 @coroutine#3,5,main]. Awoken

Although B should've slept for 150 milliseconds, it slept for almost 600 milliseconds instead.

An extra thread

#2972 raises a complaint that delay can create a new thread. It's a reasonable thing to be worried about.

#4063 also mentions that DefaultExecutor lives longer than needed occasionally.

The proposed solution

DefaultExecutor is an old thing, predating structured concurrency and our modern understanding that we're promoting everywhere: that you don't need any custom threads, just use Dispatchers.Default, Dispatchers.IO, and Dispatchers.Main, possibly with limitedParallelism, and you will be happy: there are no leaked threads, no issues with closing dispatchers, threads are shared inside a single pool. Why not apply the same logic to our internal implementation?

  • delays should be processed in the thread pool backing Dispatchers.Default and Dispatchers.IO. Specifically, it should be processed on Dispatchers.IO, as making the thread sleep is a blocking task. We will call this DefaultDelay from now on.
  • DefaultDelay should release the thread as soon as there are no delays to process, without any "keep alive" rules. Since this no longer involves the heavy operation of creating or stopping a thread, it should not cause performance issues.
  • DefaultDelay should not be responsible for cleaning up after other threads: it makes zero sense to fit that functionality into the same single thread that is also responsible for system-wide liveness. Instead, a separate view of Dispatchers.IO should be introduced internally to deal with the dropped tasks. We'll call this CleanupExecutor from now on.
  • The unconfined tasks attempting to use the event loop opened on DefaultDelay's thread must be redispatched to CleanupExecutor.
  • We should also look into whether it's possible to distinguish between dispatches backed by their own thread pool and direct (in-place) dispatches. If so, all direct dispatches should go through CleanupExecutor instead of creating non-compliant work for DefaultDelay.
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

1 participant