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

Only retry transaction once #2604

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
79 changes: 77 additions & 2 deletions lib/Doctrine/ODM/MongoDB/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@
use Doctrine\Persistence\PropertyChangedListener;
use InvalidArgumentException;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Driver\Exception\RuntimeException;
use MongoDB\Driver\Session;
use MongoDB\Driver\WriteConcern;
use ProxyManager\Proxy\GhostObjectInterface;
use ReflectionProperty;
use Throwable;
use UnexpectedValueException;

use function array_diff_key;
Expand All @@ -37,13 +39,13 @@
use function array_key_exists;
use function array_merge;
use function assert;
use function call_user_func;
use function count;
use function get_class;
use function in_array;
use function is_array;
use function is_object;
use function method_exists;
use function MongoDB\with_transaction;
use function preg_match;
use function serialize;
use function spl_object_hash;
Expand Down Expand Up @@ -460,7 +462,7 @@ public function commit(array $options = []): void

$this->lifecycleEventManager->enableTransactionalMode($session);

with_transaction(
$this->withTransaction(
$session,
function (Session $session) use ($options): void {
$this->doCommit(['session' => $session] + $this->stripTransactionOptions($options));
Expand Down Expand Up @@ -3177,4 +3179,77 @@ private function getTransactionOptions(array $options): array
self::TRANSACTION_OPTIONS,
);
}

/**
* This following method was taken from the MongoDB Library and adapted to not use the default 120 seconds timeout.
* The code within this method is licensed under the Apache License. Copyright belongs to MongoDB, Inc.
*/
Comment on lines +3184 to +3186
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what the best course of action is here, but since the code in question comes from the apache-2.0-licensed MongoDB library and the ODM is MIT licensed, we do need some kind of license and copyright information. Let me know your thoughts on this.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's OK but I'm not a lawyer :) Maybe you could ask internally if it's sufficient?

private function withTransaction(Session $session, callable $callback, array $transactionOptions = []): void
{
$closureAttempts = 0;
jmikola marked this conversation as resolved.
Show resolved Hide resolved

while (true) {
$session->startTransaction($transactionOptions);

try {
$closureAttempts++;
call_user_func($callback, $session);
} catch (Throwable $e) {
if ($session->isInTransaction()) {
$session->abortTransaction();
}

if (
$e instanceof RuntimeException &&
$e->hasErrorLabel('TransientTransactionError') &&
! $this->shouldAbortWithTransaction($closureAttempts)
) {
continue;
}

throw $e;
}

if (! $session->isInTransaction()) {
// Assume callback intentionally ended the transaction
return;
}

while (true) {
try {
$session->commitTransaction();
} catch (RuntimeException $e) {
if (
$e->getCode() !== 50 /* MaxTimeMSExpired */ &&
$e->hasErrorLabel('UnknownTransactionCommitResult') &&
! $this->shouldAbortWithTransaction($closureAttempts)
) {
// Retry committing the transaction
continue;
}

if (
$e->hasErrorLabel('TransientTransactionError') &&
! $this->shouldAbortWithTransaction($closureAttempts)
) {
// Restart the transaction, invoking the callback again
continue 2;
}

throw $e;
}

// Commit was successful
break;
}

// Transaction was successful
break;
}
}

private function shouldAbortWithTransaction(int $closureAttempts): bool
{
return $closureAttempts >= 2;
}
}
5 changes: 5 additions & 0 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,11 @@ parameters:
count: 1
path: lib/Doctrine/ODM/MongoDB/UnitOfWork.php

-
message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\UnitOfWork\\:\\:withTransaction\\(\\) has parameter \\$transactionOptions with no value type specified in iterable type array\\.$#"
count: 1
path: lib/Doctrine/ODM/MongoDB/UnitOfWork.php

-
message: "#^Unable to resolve the template type T in call to method Doctrine\\\\ODM\\\\MongoDB\\\\DocumentManager\\:\\:getClassMetadata\\(\\)$#"
count: 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,50 @@ public function testTransientInsertError(): void
self::assertEquals([], $this->uow->getDocumentChangeSet($friendUser));
}

public function testMultipleTransientErrors(): void
{
$firstUser = new ForumUser();
$firstUser->username = 'alcaeus';
$this->uow->persist($firstUser);

$secondUser = new ForumUser();
$secondUser->username = 'jmikola';
$this->uow->persist($secondUser);

$friendUser = new FriendUser('GromNaN');
$this->uow->persist($friendUser);

// Add a failpoint that triggers multiple transient errors. The transaction is expected to fail
$this->createTransientFailPoint('insert', 2);

try {
$this->uow->commit();
self::fail('Expected exception when committing');
} catch (Throwable $e) {
self::assertInstanceOf(BulkWriteException::class, $e);
self::assertSame(192, $e->getCode());
}

self::assertSame(
0,
$this->dm->getDocumentCollection(ForumUser::class)->countDocuments(),
);

self::assertSame(
0,
$this->dm->getDocumentCollection(FriendUser::class)->countDocuments(),
);

self::assertTrue($this->uow->isScheduledForInsert($firstUser));
self::assertNotEquals([], $this->uow->getDocumentChangeSet($firstUser));

self::assertTrue($this->uow->isScheduledForInsert($secondUser));
self::assertNotEquals([], $this->uow->getDocumentChangeSet($secondUser));

self::assertTrue($this->uow->isScheduledForInsert($friendUser));
self::assertNotEquals([], $this->uow->getDocumentChangeSet($friendUser));
}

public function testDuplicateKeyError(): void
{
// Create a unique index on the collection to let the second insert fail
Expand Down Expand Up @@ -534,12 +578,11 @@ protected static function getConfiguration(): Configuration
return $configuration;
}

private function createTransientFailPoint(string $failCommand): void
private function createTransientFailPoint(string $failCommand, int $times = 1): void
{
$this->dm->getClient()->selectDatabase('admin')->command([
'configureFailPoint' => 'failCommand',
// Trigger the error twice, working around retryable writes
Copy link
Member Author

Choose a reason for hiding this comment

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

While testing this change, I noticed that with a single retry all other tests started failing. Upon further investigation, it turns out that using an additional failure to account for retryable writes isn't necessary. Quoting from the Retryable Writes Documentation:

The write operations inside the transaction are not individually retryable, regardless of value of retryWrites.

With that in mind, we don't need to account for this and a single failure is sufficient for this fail point (except for the one test where we want multiple failures to specifically test that the operation is only retried once).

'mode' => ['times' => 2],
'mode' => ['times' => $times],
'data' => [
'errorCode' => 192, // FailPointEnabled
'errorLabels' => ['TransientTransactionError'],
Expand Down