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

[6.0] Fix exception handling on transaction commit in DB::transaction() #29067

Merged
merged 10 commits into from
Sep 1, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions src/Illuminate/Database/Concerns/ManagesTransactions.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ public function transaction(Closure $callback, $attempts = 1)
// catch any exception we can rollback this transaction so that none of this
// gets actually persisted to a database or stored in a permanent fashion.
try {
return tap($callback($this), function () {
$this->commit();
});
$callbackReturn = $callback($this);
}

// If we catch an exception we'll rollback this transaction and try again if we
Expand All @@ -38,11 +36,27 @@ public function transaction(Closure $callback, $attempts = 1)
$this->handleTransactionException(
$e, $currentAttempt, $attempts
);

continue;
driesvints marked this conversation as resolved.
Show resolved Hide resolved
} catch (Throwable $e) {
$this->rollBack();

throw $e;
}

// Exceptions on commit need to be handled separately as they automatically roll
// back the transaction, resulting in an error if we attempt to manually roll back.
try {
$this->commit();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic suggestion: We could move return $callbackReturn; in here and remove continue; from the catch block.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staudenmeir I considered that initially, but I think this way makes the pattern of "if something goes wrong, continue; return at the end" more clear. In fact, while working on this PR I initially forgot to put in the first continue a few lines up, and it took several minutes to figure out why I was failing unit tests. So while your way is slightly more concise, I think this way is more readable and makes the code easier to modify in the future.

} catch (Exception $e) {
$this->handleCommitTransactionException(
$e, $currentAttempt, $attempts
);

continue;
}

return $callbackReturn;
}
}

Expand Down Expand Up @@ -81,6 +95,35 @@ protected function handleTransactionException($e, $currentAttempt, $maxAttempts)
throw $e;
}

/**
* Handle an exception encountered when committing a transaction.
*
* @param \Exception $e
* @param int $currentAttempt
* @param int $maxAttempts
* @return void
*
* @throws \Exception
driesvints marked this conversation as resolved.
Show resolved Hide resolved
*/
protected function handleCommitTransactionException($e, $currentAttempt, $maxAttempts)
{
// If the COMMIT fails, the transaction is automatically rolled back.
$this->transactions--;

// If we encountered a serialization failure, try again if we are not out of attempts.
// SQLSTATE 40001 is designated as a serialization failure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, SQLSTATE 40001 is database specific .. no? I don't think this is supposed to be in this "shared" handling code.

Why is only this particular error generating this problem and not others?

Copy link
Author

@jansennerd10 jansennerd10 Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no. SQLSTATE error codes are part of the SQL standard. PostgreSQL, MySQL, SQL Server, Oracle, and IBM databases all use SQLSTATE 40001 to report serialization failures. Many databases additionally use their own error codes to provide more detailed information, but the value returned by PDOException::getCode() is the SQLSTATE code.

Any exception occurring during commit would cause the rollback problem. We do need special handling for serialization failures, though, because that is a case where (just like a deadlock) retrying the transaction is the correct course of action.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#TIL , thanks

if ($e->getCode() === '40001' &&
$currentAttempt < $maxAttempts) {
return;
}

if ($this->causedByLostConnection($e)) {
$this->transactions = 0;
}

throw $e;
}

/**
* Start a new database transaction.
*
Expand Down
32 changes: 32 additions & 0 deletions tests/Database/DatabaseConnectionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,21 @@ public function testTransactionMethodRunsSuccessfully()
$this->assertEquals($mock, $result);
}

public function testTransactionRetriesOnSerializationFailure()
{
$this->expectException(PDOException::class);
$this->expectExceptionMessage('Serialization failure');

$pdo = $this->getMockBuilder(DatabaseConnectionTestMockPDO::class)->setMethods(['beginTransaction', 'commit', 'rollBack'])->getMock();
$mock = $this->getMockConnection([], $pdo);
$pdo->method('commit')->will($this->throwException(new DatabaseConnectionTestMockPDOException('Serialization failure', '40001')));
$pdo->expects($this->exactly(3))->method('beginTransaction');
$pdo->expects($this->never())->method('rollBack');
$pdo->expects($this->exactly(3))->method('commit');
$mock->transaction(function () {
}, 3);
}

public function testTransactionMethodRetriesOnDeadlock()
{
$this->expectException(QueryException::class);
Expand Down Expand Up @@ -411,3 +426,20 @@ public function __construct()
//
}
}

class DatabaseConnectionTestMockPDOException extends PDOException
{
/**
* Overrides Exception::__construct, which casts $code to integer, so that we can create
* an exception with a string $code consistent with the real PDOException behavior.
*
* @param string|null $message
* @param string|null $code
* @return void
*/
public function __construct($message = null, $code = null)
{
$this->message = $message;
$this->code = $code;
}
}