-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
withContext and withTimeout may discard resources on cancellation #3504
Comments
We've been on this road before and decided against this behaviour, breaking quite a lot of code in the meantime: #1813 The problem is that both patterns are erroneous, but taking into account that coroutines are dominant on Android and closeable resource pattern has much less demand, I do not see a compelling reason to change this behaviour and open a whole new class of application crashes and errors after coroutines update. I believe there are other ways to resolve this issue:
|
After an internal discussion with @qwwdfsad, it turns out that the problem with the proposal is much more severe than I thought. Here's the gist of it. Basically, it's restating the "Problems with atomic cancellation" section of #1813, but expanded on and adapted a bit. On Android, there's an (unwritten?) rule that it's invalid for a coroutine running on the UI thread to do anything meaningful after it was canceled. This is due to the fact that updating a UI after it was disposed of leads to crashes. Luckily, the cancellation of UI-related coroutines also happens in the UI thread. So, typically, if some code executes in This works well. Changing withContext(Dispatchers.Main) {
val value = withContext(Dispatchers.Default) {
0
}
updateUi(value)
} The reason is, while So, this is not just about delayed results being displayed, it's about not requiring the cancellation to be cooperative and instead being mandatory in this particular case. What about |
I think scoping functions are more special, so there is no need for solution on the side of the sent value (like Possibly, the value might be delivered together with the special
The downside is that exceptions are not generic. |
Such a solution has its own downsides -- it might have been the case that WDYT about tooling-assisted help?
let's suggest an intention (with a quickfix) to replace it with the following:
|
|
I don't get this argument. How a |
Hi all, For developers like me, learning Kotlin, this sample is even more of a problem than it looks. Normally, the acquisition and the release of resources can be non-cancellable. So, this seems to work (and I had to test it, to see it actually working): // NOTE: I don't understand why this works, as it's also making use of `withContext`,
// but I'm guessing that the `NonCancellable` context will hide the cancellation status.
val socket = withContext(NonCancellable) {
withContext(Dispatchers.IO) {
Socket("localhost", 8080)
}
}
socket.use {
// ...
} It requires training for users, but as a mental model, you can just say that acquisition and release have to be NON-CANCELLABLE, most of the time, and so as a user you need to ensure that they are non-cancellable. This is an easy rule to learn, especially once you get burned once or twice. The issue here is that this Another use-case for a cancellable acquisition is that of a lock: lock.acquire()
try {
//...
} finally {
lock.release()
} Which could make use of Kotlin's coroutines: suspend fun <T> CoroutineScope.withLock(
lock: AtomicBoolean,
block: suspend CoroutineScope.() -> T
): T {
runInterruptible(Dispatchers.IO) {
while (!lock.compareAndSet(false, true)) {
Thread.onSpinWait()
if (Thread.interrupted())
throw InterruptedException()
}
}
try {
return block(this)
} finally {
lock.set(false)
}
} The behaviour of this code for me, as a beginner in Kotlin, is very unintuitive because my mental model for what can be cancelled is invalidated. For example, I assumed that And fixing this particular sample is even more difficult, as the release of the resource shouldn't be done twice: suspend fun <T> CoroutineScope.withLock(
lock: AtomicBoolean,
block: suspend CoroutineScope.() -> T
): T {
var isLocked = false
var throwFromUserCode = false
try {
runInterruptible(Dispatchers.IO) {
while (!lock.compareAndSet(false, true)) {
Thread.onSpinWait()
if (Thread.interrupted())
throw InterruptedException()
}
isLocked = true
}
try {
throwFromUserCode = true
return block(this)
} finally {
lock.set(false)
}
} catch (e: Throwable) {
if (!throwFromUserCode && isLocked)
lock.set(false)
throw e
}
} In my opinion™️, resource acquisition/release (and cancellation/interruption in general) is very unsafe if you don't know which instruction is guaranteed to execute next. In Java/Kotlin, we can think of Furthermore, IMO, while I would love to see a universal ability to allocate/deallocate resources in Kotlin, tied to the current scope, this won't fix the fact that the boundaries of what's cancellable and what's not is unclear, given the prevalence of PS: Kotlin's Coroutines are wonderful, thank you so much for your contributions! ❤️ PPS: I have a JBang script to show the deadlock, maybe this is useful for others that want to play: ///usr/bin/env jbang "$0" "$@" ; exit $?
//JAVA 17+
//KOTLIN 1.8.20
//DEPS org.jetbrains.kotlinx:kotlinx-coroutines-core:1.7.0-RC
import java.util.concurrent.atomic.AtomicBoolean
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.runInterruptible
fun main() = runBlocking {
val lock = AtomicBoolean(false)
repeat(10000) { index ->
val job = launch {
println("Starting job $index...")
withLock(lock) {
delay(1)
println("Job $index done.")
}
}
if (index % 2 == 0)
launch {
println("Cancelling job $index")
try {
job.cancelAndJoin()
} catch (_: CancellationException) {}
}
}
}
suspend fun <T> CoroutineScope.withLock(
lock: AtomicBoolean,
block: suspend CoroutineScope.() -> T
): T {
return this.withLockLeaky(lock, block)
// return this.withLockNonLeaky(lock, block)
}
suspend fun <T> CoroutineScope.withLockLeaky(
lock: AtomicBoolean,
block: suspend CoroutineScope.() -> T
): T {
runInterruptible(Dispatchers.IO) {
while (!lock.compareAndSet(false, true)) {
Thread.onSpinWait()
if (Thread.interrupted())
throw InterruptedException()
}
}
try {
return block(this)
} finally {
lock.set(false)
}
}
suspend fun <T> CoroutineScope.withLockNonLeaky(
lock: AtomicBoolean,
block: suspend CoroutineScope.() -> T
): T {
var isLocked = false
var throwFromUserCode = false
try {
runInterruptible(Dispatchers.IO) {
while (!lock.compareAndSet(false, true)) {
Thread.onSpinWait()
if (Thread.interrupted())
throw InterruptedException()
}
isLocked = true
}
try {
throwFromUserCode = true
return block(this)
} finally {
lock.set(false)
}
} catch (e: Throwable) {
if (!throwFromUserCode && isLocked)
lock.set(false)
throw e
}
} |
…ress indicator is shown" This reverts commit d2576dd2d846daa0b022e91991660fee85f03280. Consider this piece: ``` withContext(Dispatchers.EDT) { showIndicatorInUI(project, taskInfo, indicator) } ``` `showIndicatorInUI` was completed but `withContext` resumed with `CancellationException` if the coroutine is cancelled concurrently, which happens if the task takes about 300ms and completes while `showIndicatorInUI` is executed. `CancellationException` from `withContext` prevented `indicator.finish`. `withContext` resuming with CE even if block completed without CE is tracked here: Kotlin/kotlinx.coroutines#3504 GitOrigin-RevId: e684335dcb7bb2eb8abb7399eec7cdf1788470d2
This "obviously correct" code has a bug: if cancellation happens after the last suspension point in the
withContext
block, the resource will be successfully created, but it will not be released, becausewithContext
will throw aCancellationException
after executing the block.The same issue can be observed with
withTimeout
.The proper way to write the code above is:
This is very error-prone, and it's quite confusing why the first version is buggy while the second one is not.
A simple solution is to make
withContext
avoid checking whether the coroutine was cancelled if a value was obtained and instead always return the value.A counterargument against this solution is that this is an edge case that is likely immensely rare, but changing the behavior of
withContext
andwithTimeout
is a breaking change that affects the use cases that are arguably much more common. For example:If
doAVeryLongComputation
is not cancellable andwithContext
returns a value instead of throwing an exception, thenupdateUiElement
may happen long after a cancellation is requested. To fix such code, one would have to door make
doAVeryLongComputation
itself cancellable.The text was updated successfully, but these errors were encountered: