Coroutines and deadlocks via transactions across suspend functions

[ursusursus]

Hi, so as per Room developers article https://medium.com/androiddevelopers/threading-models-in-coroutines-and-android-sqlite-api-6cab11f7eb90

running transactions across suspending functions might end in deadlocks, since only 1 concurrent transaction per connection is allowed

sqldelight doesnt expose suspend api, which saves it, i.e. transactions will most likely run regular synchronous code (wrapped by a suspend repository / store function for example)

however nesting these suspend repository / store functions (transactions) might end unexpectedly in tears

Is something needed to be done here?

[erikc5000]

I was thinking about this too after reading the article. I could be wrong, but I don't think you can get yourself in trouble with this in SqlDelight though?

Since the transaction blocks aren't suspending, you'd have to get another coroutine running to execute code in a different thread, but then any transactions or queries that coroutine performs would be confined to that other thread and block if necessary if there's an ongoing transaction. Even if you're running your transactions on a dispatcher with multiple threads, I'm not seeing a way that you can get a thread switch to happen mid-transaction within a particular coroutine -- unless I'm missing something.

[JakeWharton]

Yeah I don't think there's anything to do here. Room dug itself into that hole and are having to engineer complicated workarounds because of those choices. SQL Delight's semantics are exactly that of the underlying blocking APIs and you cannot call start and end transaction on different threads or have code easily or un-obviously running on a different thread within a transaction.

[ursusursus]

@eric Yes library it self seems ok, but usually people wrap XQueries into a Dao / Store / whatever and have those functions be suspending

And therefore could accidentally nest transactions like this

suspend fun syncUsersAndMessages(users, messages) {
   transacter.inTransaction {
      syncUsers(users)
      syncMessages(messages)
   }
}

suspend fun syncUsers(users)...

suspend fun syncMessages(messages)

This would be valid api (before coeoutines migration) as per validness of nesting transaction, and I feel people will just slap suspend on these functions without thinking, whereas they'd have to think if these were Completables

[erikc5000]

@ursusursus: That code won't compile as written though. The calls to syncUsers() and syncMessages() require a coroutine context. You'd have to surround them with runBlocking {} or launch another coroutine, which avoids the issue in question -- though launching a separate coroutine would cause the transaction to have no effect.

[ursusursus]

I think it would compile afaik, context would be the one of the caller, but I just realized you' right in the above messages, its just blocking code, suspend keyword doest make it async magicly

[AlecKazakova]

boop bopp if theres more here feel free to reopen

[strooooke]

Not necessarily more, but as a voice from the user base: I came here precisely because in our project this helper turned up

class DbHelper(val coroutineScope: CoroutineScope) {
    private val database = ... holds a database instance...

    /**
     * Joins or starts a database based transaction.
     */
    override suspend fun <T> newOrJoinTx(block: suspend () -> T): T = withContext(coroutineDispatcher) {
        val result: Deferred<T> = database.transactionWithResult {
            async {
                block()
            }
        }
        result.await()
    }
}

Thankfully, this was detected in review.

Time to re-architect our approach. It would be really really nice if we could have something like in room.

[morki]

I think this need to be reopen, we ran into silent bug in production because of this. We generateAsync = true for sqlite JDBC driver and using generated suspend methods. We use this approach because of possible migration to R2DBC Postgres driver in future, to be prepared for async.

We use code like this:

database.transaction {
    batch.entities.collect { entity ->
        database.entityQueries.create(id = entity.id, /*...*/)
    }

    database.batchQueries.create(date = batch.date, processedAt = Clock.System.now())
}

Some queries entityQueries.create() run outside of the transaction, because they silently use other Dispatcher / Thread underneath.