-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Open
Description
If withTimeout
wraps a blocking job, it does not detect a timeout in conjunction with newSingleThreadContext()
. Timeout detection works as expected with other dispatcher flavors:
import kotlinx.coroutines.* // ktlint-disable no-wildcard-imports
import java.util.concurrent.Executors
@OptIn(ExperimentalCoroutinesApi::class, DelicateCoroutinesApi::class)
fun main(): Unit = runBlocking {
listOf(
Triple("newSingleThreadExecutor", { Executors.newSingleThreadExecutor().asCoroutineDispatcher() }, true),
Triple("newSingleThreadContext", { newSingleThreadContext("single") }, true),
Triple("newFixedThreadPoolContext(2)", { newFixedThreadPoolContext(2, "double") }, true),
Triple("IO.limitedParallelism(1)", { Dispatchers.IO.limitedParallelism(1) }, false),
Triple("IO.limitedParallelism(2)", { Dispatchers.IO.limitedParallelism(2) }, false)
).forEach { (name, dispatcher, needsClosing) ->
print("$name: ")
val dispatcher = dispatcher()
try {
withContext(dispatcher) {
try {
withTimeout(1000) {
launch {
Thread.sleep(2000)
// delay(2000)
}
}
println("no timeout detected")
} catch (t: Throwable) {
println("$t")
}
}
} finally {
if (needsClosing) (dispatcher as ExecutorCoroutineDispatcher).close()
}
}
}
produces:
newSingleThreadExecutor: kotlinx.coroutines.TimeoutCancellationException: Timed out waiting for 1000 ms
newSingleThreadContext: no timeout detected
newFixedThreadPoolContext(2): kotlinx.coroutines.TimeoutCancellationException: Timed out waiting for 1000 ms
IO.limitedParallelism(1): kotlinx.coroutines.TimeoutCancellationException: Timed out waiting for 1000 ms
IO.limitedParallelism(2): kotlinx.coroutines.TimeoutCancellationException: Timed out waiting for 1000 ms
Version: kotlinx-coroutines-core:1.7.3
Effects observed here first: kotest/kotest#3672
sschuberth
Activity
dkhalanskyjb commentedon Sep 4, 2023
What are you trying to achieve?
withTimeout
does not interrupt blocking code as is, you needrunInterruptible
(https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/run-interruptible.html) for that. The only difference is whether, on a timeout, an exception will be thrown after the code finishes executing.OliverO2 commentedon Sep 4, 2023
That's of course all valid and has been noted in kotest/kotest#3672 (comment) and kotest/kotest#3672 (comment).
What the original test in question was trying to achieve can probably be best answered by @sschuberth.
The point here is that the timeout exception after the blocking section is usually available, but missing with one type of dispatcher. From a Kotest point of view, this change in behavior is considered a regression.
sschuberth commentedon Sep 4, 2023
The code where this Kotest regression occurred is here, the test was written by @oheger-bosch. My interpretation is that the test should ensure that a call to
fossId.scanPackage()
blocks, and this is being tested by calling itwithTimeout
, expecting that we run into aTimeoutCancellationException
.OliverO2 commentedon Sep 4, 2023
Came up with another scenario: What happens if the timeout just occurs during a cpu-bound period, even if the coroutine is non-blocking otherwise? Seems like in this case
newSingleThreadContext()
also misses the timeout:produces
dkhalanskyjb commentedon Sep 4, 2023
Suggestion:
This way, there's no illusion that the blocking code gets canceled.
In this case, you're supposed to call
ensureActive
periodically during your work. Otherwise, there's no way forwithTimeout
to cancel the block of code.OliverO2 commentedon Sep 4, 2023
(In my own code, 'm actually using
isActive
andyield()
in CPU-bound blocks.)Regarding this case, as a library user, without looking into the actual kotlinx.coroutines code, my mental model is as follows:
withTimeout
wraps a block likecoroutineScope
does.joinAll()
for coroutines launched in that block.joinAll()
to be the place where the cancellation would happen (and the timeout be detected for blocking jobs).If that is conceptually correct: If a job, which is waited for, happens to be consuming the CPU for a very brief period at the end, and in exactly that period the timeout expires, doesn't that seem racy?
Note that I'm not expecting a cancellation to interrupt that (briefly) blocking job. Just to detect the timeout.
sschuberth commentedon Sep 4, 2023
Thanks, but how you avoid the test to really take "a long time" then? The test case should complete as quickly as possible.
OliverO2 commentedon Sep 4, 2023
In this case you should use
runInterruptible
or the corresponding Kotest blocking/timeout configuration.sschuberth commentedon Sep 4, 2023
I just looked at the implementation for Kotest's
shouldTimeout
, and it turns out it does the same thing that we did manually, seehttps://github.com/kotest/kotest/blob/402597ca4bf581f10df2f3d062a2427e0de2d005/kotest-assertions/kotest-assertions-shared/src/jvmMain/kotlin/io/kotest/assertions/async/timeout.kt#L18-L26
So I guess this needs some fixing as well?
dkhalanskyjb commentedon Sep 4, 2023
If it's for running suspending code, it seems perfectly fine. The problem is only about code that doesn't participate in cooperative cancellation.
test(fossid): Improve timeout testing for blocking code
6 remaining items