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
Caught deadlock exceptions cause transaction level to remain the same #32543
Comments
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. |
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 |
We've actually tried to "fix" this at one point. Here's the issue: #30756 But we had to revert because it caused problems for other people: ppy/osu-web#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. |
@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. |
Thanks. What do you recommend as a fix? |
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:
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: Normally, you would expect this line to rollback savepoint And that's not what most developers would expect to happen. To battle this, we could introduce a $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 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 |
@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. |
@oprypkhantc have you been able to look into a pr yet? |
@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. |
Closing this as there's not much movement here anymore. Feel free to send in a PR if you want. |
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:
php artisan queue:work
It will throw
PDOException : SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT trans2 does not exist
when it obviously shouldn't have.The text was updated successfully, but these errors were encountered: