newSuspendedTransaction doesn't work after failure

[davydes]

Example:

runBlocking {
  try {
    newSuspendedTransaction {
      println("Try to insert...")
      throw RuntimeException("Fail")
    }
  } catch (i: Exception) {
    println("Seems record exists, then...")
  }

  newSuspendedTransaction {
    println("Try to update...")
    println("Success")
  }
}

Produces output:

Try to insert...
Seems record exists, then...

[Tapac]

Hi @davydes , it's a normal behavior as the whole runBlocking scope is interrupted when the exception is thrown. Please read more about exceptions and coroutines here or here

[davydes]

Hi @davydes , it's a normal behavior as the whole runBlocking scope is interrupted when the exception is thrown. Please read more about exceptions and coroutines here or here

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.

[davydes]

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:

Try to insert...
Seems record exists, then...
Updating...
Success!

Why newSuspendedTransaction works in different way?

[Tapac]

The same here with newSuspendedTransaction if you don't provide a specific context it will be executed within current and when failed the whole context execution will be prevented. You could provide the context as a first parameter like newSuspendedTransaction(IO) {} then your code will run as expected

[nomisRev]

Hi @davydes , it's a normal behavior as the whole runBlocking scope is interrupted when the exception is thrown. Please read more about exceptions and coroutines here or here

@Tapac,If I'm not mistaken this breaks the semantics of Structured Concurrency as it's followed by Kotlinx Coroutines. If newSuspendedTransaction cancels the parent, then it should also throw CancellationException.

Looking at the code, https://github.com/JetBrains/Exposed/blob/990bff364176716e92cc1aac3a0217940da96513/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/transactions/experimental/Suspended.kt#L21 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")
  }
}

1
Failure(java.lang.IllegalStateException: Boom!)
2

Additionally, I don't think providing a different CoroutineDispatcher should be able to influence this behavior at all 🤔 Since the same parent reference should still hold, since:

val parent = Job()
assertEquals((parent + Dispatchers.IO)[Job], parent)

[j30ng]

@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 {
            // ...
        }
    }
}

[nomisRev]

because it cancels the parent in case of an unhandled exception.

The exceptions is handled in both the original and my example, so it should not have cancelled the parent? 🤔 The parent being runBlocking. This doesn't follow the same semantics of KotlinX Coroutines, where newSuspendedTransaction should behave the same as withContext or coroutineScope.

Can you post a complete example that shows the behavior? Perhaps I misunderstood you.

[j30ng]

The exceptions is handled in both the original and my example, so it should not have cancelled the parent? 🤔 The parent being runBlocking. This doesn't follow the same semantics of KotlinX Coroutines, where newSuspendedTransaction should behave the same as withContext or coroutineScope.

It depends on which kinds of coroutines you are using. coroutineScope does not propagate up its exceptions - it just rethrows them, so you can simply wrap it with a try-catch to handle exceptions occurred inside it. launch and async however do propagate up their exceptions, causing their parents to be cancelled.

Can you post a complete example that shows the behavior? Perhaps I misunderstood you.

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

1
Failure(java.lang.IllegalStateException: Boom!)
2

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 async propagates up and cancels its parents. But if you replace the first coroutineScope with supervisorScope, like:

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.

[nomisRev]

@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 newSuspendedTransaction has the same signature as coroutineScope or withContext so I expect it to behave the same. Further more, I do not expect that I have to wrap it into supervisorScope to run it if I want to catch any exceptions.

If not, I'd expect it to actually work like async which is of shape fun CoroutineScope.async(...): Deferred<T>. The reason that async works the way you describe is because it's not a suspend function and is thus unscoped through suspend unlike coroutineScope or newSuspendedTransaction.

This can be shown by commenting await in your example, meaning that the exceptions has to be caught inside async itself if you want 2 to be printed.

fun main() = runBlocking {
    coroutineScope {
        kotlin.runCatching {
            async {
                println("1")
                throw IllegalStateException("Boom!")
            }//.await()
        }.let(::println)
    }

    println("2")
}

Still prints (and same if we replace async with launch).

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:

suspend fun <A> newSuspendedTransaction(..): A should in my opinion behave like suspend fun <A> withContext(...): A or suspend fun <A> coroutineScope(..): A.

If it's desired to behave like fun <A> CoroutineScope.async(...): Deferred<A> or fun <A> launch(...): A I think it should match the same signature, fun <A> CoroutineScope.newSuspendedTransaction(...): Deferred<A>, but I think that is much less desirable. Otherwise I feel like we're comparing oranges to apples if we are not comparing functions with the same signatures.

[matejdro]

Agree here, if I already catch the exception, then that exception should not propagate to the parent scope. This breaks usual coroutine behavior.

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)
   }
}