Framework: Caught deadlock exceptions cause transaction level to remain the same

[oprypkhantc]

• Laravel Version: 7.5.2
• PHP Version: 7.4.2
• Database Driver & Version: mysql 5.7

Description:

\Illuminate/Database/Connection (later on - database connection) relies on catching deadlock exceptions to decrement transaction level. If these exceptions are caught by developer and not re-thrown, database connections will not be aware of the errors and transaction level will remain the same, when it should have been decremented.

Regardless of whether developer wants to catch those exceptions or not, DBMS will decrement the transaction level, so we need to do the same. If any errors occurs anywhere in database connection,
we have to decrement the transaction level.

Steps To Reproduce:

1. Clone https://github.com/autaut03/deadlock-transaction-level-bug
2. Configure database connection & any queue driver (not sync), migrate
3. Run queue worker: php artisan queue:work
4. With queue worker still running, run test https://github.com/autaut03/deadlock-transaction-level-bug/blob/master/tests/Unit/DecrementsTransactionLevelEvenIfCaughtExceptionTest.php

It will throw PDOException : SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT trans2 does not exist when it obviously shouldn't have.

[oprypkhantc]

Just found out I can not reproduce it locally, but the problem does exist as we've encountered it multiple times on production. Will provide a more complete test case as soon as I have more information.

[autaut03]

Turns out I could not reproduce it locally because I was getting lock wait timeout, not a deadlock. Here's a complete reproduction: https://github.com/autaut03/deadlock-transaction-level-bug/blob/master/tests/Unit/DecrementsTransactionLevelEvenIfCaughtExceptionTest.php

[driesvints]

We've actually tried to "fix" this at one point.

Here's the issue: https://github.com/laravel/framework/issues/30756
Here's the PR: https://github.com/laravel/framework/pull/30883

But we had to revert because it caused problems for other people: https://github.com/ppy/osu-web/issues/5452

At this point we won't be reconsidering to change this behavior again unless someone can provide a bulletproof way that doesn't breaks other people's use cases.

[oprypkhantc]

@driesvints Correct me if I'm wrong, but my issue isn't one you've mentioned? The issue you've mentioned talks about TransactionCommitted event and I'm having issues with Laravel not being aware of transaction level decrease on deadlocks. I don't see how those are related.

Though you are right the fix would be a breaking change, I don't see why can't this be fixed in Laravel 8.0/9.0.

[driesvints]

Thanks. What do you recommend as a fix?

[oprypkhantc]

This is a breaking change for rare cases like ours, but overall it shouldn't affect 99% of transactions' use cases.

Ideally, you would want to:

1. Drop concurrency error handling from https://github.com/laravel/framework/blob/7.x/src/Illuminate/Database/Concerns/ManagesTransactions.php#L71
2. Make sure DetectsConcurrencyErrors trait only checks for deadlocks and ignore lock wait timeouts, as those do not cause mysql to do anything and hence do not need to be handled: https://github.com/laravel/framework/blob/7.x/src/Illuminate/Database/DetectsConcurrencyErrors.php#L33
3. Add deadlock error handling to https://github.com/laravel/framework/blob/7.x/src/Illuminate/Database/Connection.php#L720 :

protected function handleQueryException(QueryException $e, $query, $bindings, Closure $callback)
{
    if (isDeadlock($e)) {
        $this->transactions = 0;

        throw $e;
    }

    if ($this->transactions >= 1) {
        throw $e;
    }

    return $this->tryAgainIfCausedByLostConnection(
        $e, $query, $bindings, $callback
    );
}

This will fix the problem, but that's not the whole story. Since we would now be decrementing transaction level under the hood regardless of whether exception is caught by developer or not, caught deadlock exceptions would cause unexpected flow:

https://github.com/autaut03/deadlock-transaction-level-bug/blob/master/tests/Unit/DecrementsTransactionLevelEvenIfCaughtExceptionTest.php#L70

Normally, you would expect this line to rollback savepoint trans2 - that's exactly what would happen if no deadlocks were occurred. However, if deadlock does happen, this line will rollback the transaction, not the savepoint, as transaction level has already been set to 0 by the moment it tries to rollback.

And that's not what most developers would expect to happen. To battle this, we could introduce a Transaction class, which is aware of it's own level and hence can commit only if current connection's level is same or greater, otherwise throw an exception:

$outerTransaction = DB::newTransaction();
// Transaction has begun and $outerTransaction knows it's level is 1

// Create a savepoint
$innerTransaction = DB::newTransaction();
// Savepoint was created and $innerTransaction knows it's level is 2

$caughtException = null;

try {
    // Cause a deadlock on this connection
    User::whereKey($secondUserKey)
        ->update([
            'id' => $secondUserKey,
        ]);
} catch (Throwable $e) {
    $caughtException = $e;
}

$this->assertNotNull($caughtException);
$this->assertInstanceOf(PDOException::class, $caughtException);

// will not execute any queries as transaction level is already 0 at this point and
// $innerTransaction knows it should rollback to level 2
$innerTransaction->rollBack();

// should throw an exception as this can not be committed
$outerTransaction->commit();

In most cases Connection::transaction() would still be a preferred choice, but when more control is needed you would use Connection::newTransaction(). Manual control over connection's transaction level using Connection::beginTransaction(), Connection::commit() and Connection::rollBack() would be discouraged in favour of Connection::newTransaction(), though still possible and remain the same.

The concept isn't new, obviously; for example, it's used by Ktorm, a popular Kotlin ORM: https://ktorm.liuwj.me/en/transaction-management.html#Transaction-Manager

[driesvints]

@oprypkhantc I think at this point the best thing you can do is send in a PR to master to see what Taylor says. Thanks for the detailed post.

[driesvints]

@oprypkhantc have you been able to look into a pr yet?

[oprypkhantc]

@driesvints Hey. Yeah, I have a branch ready, though I still have to tinker some things to make sure it works correctly under all conditions. I'll make a PR later today.

[driesvints]

Closing this as there's not much movement here anymore. Feel free to send in a PR if you want.