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

newSuspendedTransaction doesn't work after failure #1075

Open
davydes opened this issue Oct 21, 2020 · 10 comments
Open

newSuspendedTransaction doesn't work after failure #1075

davydes opened this issue Oct 21, 2020 · 10 comments
Labels
waiting for reply Additional information required

Comments

@davydes
Copy link

davydes commented Oct 21, 2020

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
Copy link
Contributor

Tapac commented Oct 21, 2020

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
Copy link
Author

davydes commented Oct 22, 2020

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
Copy link
Author

davydes commented Oct 22, 2020

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
Copy link
Contributor

Tapac commented Oct 22, 2020

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

@Tapac Tapac added the waiting for reply Additional information required label Apr 11, 2021
@nomisRev
Copy link

nomisRev commented Apr 11, 2023

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,

internal class TransactionScope(internal val tx: Lazy<Transaction>, parent: CoroutineContext) : CoroutineScope, CoroutineContext.Element {
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
Copy link

j30ng commented Sep 4, 2023

@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
Copy link

nomisRev commented Sep 4, 2023

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
Copy link

j30ng commented Sep 4, 2023

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
Copy link

nomisRev commented Sep 6, 2023

@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
Copy link

matejdro commented Jul 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for reply Additional information required
Projects
None yet
Development

No branches or pull requests

5 participants