Skip to content

PHPLIB-1531: Replace FailPointObserver with different logic #1427

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 20, 2024
Merged

PHPLIB-1531: Replace FailPointObserver with different logic #1427

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63d8d4f
Replace FailPointObserver with different logic
alcaeus Sep 19, 2024
b142235
Extract fail point management to a trait used by Context
jmikola Sep 20, 2024
8921152
Ensure event listeners are still unregistered after an operation fails
jmikola Sep 20, 2024
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
18 changes: 18 additions & 0 deletions tests/UnifiedSpecTests/Context.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use LogicException;
use MongoDB\Client;
use MongoDB\Driver\ClientEncryption;
use MongoDB\Driver\Server;
use MongoDB\Driver\ServerApi;
use MongoDB\Model\BSONArray;
use MongoDB\Tests\FunctionalTestCase;
Expand Down Expand Up @@ -60,6 +61,9 @@ final class Context

private ?object $advanceClusterTime = null;

/** @var list<array{failPoint: stdClass, server: Server}> */
private array $failPoints = [];

public function __construct(Client $internalClient, string $uri)
{
$this->entityMap = new EntityMap();
Expand Down Expand Up @@ -232,6 +236,20 @@ public function stopEventCollectors(): void
}
}

public function registerFailPoint(stdClass $failPoint, Server $server): void
{
$this->failPoints[] = [
'failPoint' => $failPoint,
'server' => $server,
];
}

/** @return list<array{failPoint: stdClass, server: Server}> */
public function getFailPoints(): array
{
return $this->failPoints;
}

/** @param string|array $readPreferenceTags */
private function convertReadPreferenceTags($readPreferenceTags): array
{
Expand Down
63 changes: 0 additions & 63 deletions tests/UnifiedSpecTests/FailPointObserver.php
View file
Open in desktop

This file was deleted.

13 changes: 10 additions & 3 deletions tests/UnifiedSpecTests/Operation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,16 +909,15 @@ private function executeForTestRunner()
assertArrayHasKey('failPoint', $args);
assertInstanceOf(Client::class, $args['client']);
assertInstanceOf(stdClass::class, $args['failPoint']);
$args['client']->selectDatabase('admin')->command($args['failPoint']);
$this->createFailPoint($args['failPoint'], $args['client']->getManager()->selectServer());
break;
case 'targetedFailPoint':
assertArrayHasKey('session', $args);
assertArrayHasKey('failPoint', $args);
assertInstanceOf(Session::class, $args['session']);
assertInstanceOf(stdClass::class, $args['failPoint']);
assertNotNull($args['session']->getServer(), 'Session is pinned');
$operation = new DatabaseCommand('admin', $args['failPoint']);
$operation->execute($args['session']->getServer());
$this->createFailPoint($args['failPoint'], $args['session']->getServer());
break;
case 'loop':
assertArrayHasKey('operations', $args);
Expand All @@ -937,6 +936,14 @@ private function executeForTestRunner()
}
}

private function createFailPoint(stdClass $failPoint, Server $server): void
{
$operation = new DatabaseCommand('admin', $failPoint);
$operation->execute($server);

$this->context->registerFailPoint($failPoint, $server);
}

private function getIndexNames(string $databaseName, string $collectionName): array
{
return array_map(
Expand Down
33 changes: 19 additions & 14 deletions tests/UnifiedSpecTests/UnifiedTestRunner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ final class UnifiedTestRunner
/** @var callable(EntityMap):void */
private $entityMapObserver;

private ?FailPointObserver $failPointObserver = null;

private ServerParameterHelper $serverParameterHelper;

public function __construct(string $internalClientUri)
Expand Down Expand Up @@ -150,9 +148,6 @@ private function doSetUp(): void
* after transient error within a transaction" pinning test causes the
* subsequent transaction test to block. */
$this->killAllSessions();

$this->failPointObserver = new FailPointObserver();
$this->failPointObserver->start();
}

private function doTearDown(bool $hasFailed): void
Expand All @@ -163,9 +158,6 @@ private function doTearDown(bool $hasFailed): void
$this->killAllSessions();
}

$this->failPointObserver->stop();
$this->failPointObserver->disableFailPoints();

/* Manually invoking garbage collection since each test is prone to
* create cycles (perhaps due to EntityMap), which can leak and prevent
* sessions from being released back into the pool. */
Expand Down Expand Up @@ -216,13 +208,17 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn
$context->startEventObservers();
$context->startEventCollectors();

foreach ($test->operations as $o) {
$operation = new Operation($o, $context);
$operation->assert();
}
try {
foreach ($test->operations as $o) {
$operation = new Operation($o, $context);
$operation->assert();
}

$context->stopEventObservers();
$context->stopEventCollectors();
$context->stopEventObservers();
$context->stopEventCollectors();
} finally {
$this->disableFailPoints($context->getFailPoints());
}

if (isset($test->expectEvents)) {
assertIsArray($test->expectEvents);
Expand All @@ -235,6 +231,15 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn
}
}

/** @param list<array{failPoint: stdClass, server: Server}> $failPoints */
private function disableFailPoints(array $failPoints): void
{
foreach ($failPoints as ['failPoint' => $failPoint, 'server' => $server]) {
$operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']);
$operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint->configureFailPoint, 'mode' => 'off']);

@alcaeus: This should resolve the runtime error you encountered.

That said, I came up with another PR that consolidates this logic into a ManagesFailPointsTrait, which is closer to what we had with FailPointObserver. These files are already quite complex, so I wanted to minimize additional clutter. I'll defer to you, though.

See: alcaeus#1

$operation->execute($server);
}
}

/**
* Checks server version and topology requirements.
*
Expand Down
Loading