-
Notifications
You must be signed in to change notification settings - Fork 683
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
newSuspendedTransaction doesn't work after failure #1075
Comments
But seems it breaks logic, i didn't run any job here, I just run coroutine and handle exception which it throws. Why it interrupts my parent CoroutineScope? In links you provided explanation shows only case when I run launch with unhandled exception inside. |
Also I can rewrite code: runBlocking {
try {
withContext(IO) {
transaction {
println("Try to insert...")
throw RuntimeException("Fail")
}
}
} catch (i: Exception) {
println("Seems record exists, then...")
}
withContext(IO) {
transaction {
println("Updating...")
println("Success!")
}
}
} And it will produce next output:
Why newSuspendedTransaction works in different way? |
The same here with |
@Tapac,If I'm not mistaken this breaks the semantics of Structured Concurrency as it's followed by Kotlinx Coroutines. If Looking at the code, Line 21 in 990bff3
TransactionScope creates a new CoroutineScope with the runBlocking as parent. Which is effectively the same as withContext or coroutineScope , but these builders behave entirely differently.
runBlocking {
kotlin.runCatching {
coroutineScope {
println("1")
throw IllegalStateException("Boom!")
}
}.let(::println)
coroutineScope {
println("2")
}
}
Additionally, I don't think providing a different val parent = Job()
assertEquals((parent Dispatchers.IO)[Job], parent) |
@nomisRev To be fair, this behavior rather complies with the semantics of structured concurrency, because it cancels the parent in case of an unhandled exception. Try opening supervisorScope around newSuspendedTransaction, like: try {
supervisorScope {
newSuspendedTransaction {
// ...
}
}
} |
The exceptions is handled in both the original and my example, so it should not have cancelled the parent? 🤔 The parent being Can you post a complete example that shows the behavior? Perhaps I misunderstood you. |
It depends on which kinds of coroutines you are using.
Sure, from your code snippet above (slightly modified) runBlocking {
coroutineScope {
kotlin.runCatching {
coroutineScope {
println("1")
throw IllegalStateException("Boom!")
}
}.let(::println)
}
coroutineScope {
println("2")
}
} indeed prints
but consider: runBlocking {
coroutineScope {
kotlin.runCatching {
// coroutineScope {
async {
println("1")
throw IllegalStateException("Boom!")
}.await()
// }
}.let(::println)
}
coroutineScope {
println("2")
}
} This doesn't print 2 at all, because the exception thrown from runBlocking {
// coroutineScope {
supervisorScope {
kotlin.runCatching {
async {
println("1")
throw IllegalStateException("Boom!")
}.await()
}.let(::println)
}
coroutineScope {
println("2")
}
} Now 2 gets printed at the end, because supervisorScope stops the exception propagation. These are just all normal Kotlin coroutine behaviors, and a part of structured concurrency. |
@j30ng thank you for the example. To rephrase my original comment, and I think I'm still in line with the original bug report. Semantically it still doesn't seem sense to me, because If not, I'd expect it to actually work like This can be shown by commenting fun main() = runBlocking {
coroutineScope {
kotlin.runCatching {
async {
println("1")
throw IllegalStateException("Boom!")
}//.await()
}.let(::println)
}
println("2")
} Still prints (and same if we replace Success(DeferredCoroutine{Active}@63e31ee)
1
Exception in thread "main" java.lang.IllegalStateException: Boom!
at my.timer.TreeKt$main$1$1$1$1.invokeSuspend(tree.kt:42)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:280)
at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:85)
at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:59)
at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source)
at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:38)
at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source)
at my.timer.TreeKt.main(tree.kt:37)
at my.timer.TreeKt.main(tree.kt) So to recap my thoughts:
If it's desired to behave like |
Agree here, if I already Here is a more fleshed version of above workaround that we are using in our code: /**
* Wrapper for [newSuspendedTransaction] that fixes error propagation.
* A workaround for https://github.com/JetBrains/Exposed/issues/1075.
*/
suspend fun <T> newSuspendedTransactionWithExceptionHandling(
context: CoroutineContext? = null,
db: Database? = null,
transactionIsolation: Int? = null,
statement: suspend Transaction.() -> T
): T {
return supervisorScope {
newSuspendedTransaction(context, db, transactionIsolation, statement)
}
} |
Example:
Produces output:
The text was updated successfully, but these errors were encountered: