Skip to content

ManagesFailPointsTrait and always stop event listeners #1

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 2 commits into from
Sep 20, 2024
Merged

ManagesFailPointsTrait and always stop event listeners #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7615c76
Extract fail point management to a trait used by Context
jmikola Sep 20, 2024
8d7d992
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
20 changes: 2 additions & 18 deletions tests/UnifiedSpecTests/Context.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
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 @@ -39,6 +38,8 @@
*/
final class Context
{
use ManagesFailPointsTrait;

private ?string $activeClient = null;

private EntityMap $entityMap;
Expand All @@ -61,9 +62,6 @@ 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 @@ -236,20 +234,6 @@ 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
40 changes: 40 additions & 0 deletions tests/UnifiedSpecTests/ManagesFailPointsTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace MongoDB\Tests\UnifiedSpecTests;

use MongoDB\Driver\Server;
use MongoDB\Operation\DatabaseCommand;
use stdClass;

use function PHPUnit\Framework\assertIsString;
use function PHPUnit\Framework\assertObjectHasAttribute;

trait ManagesFailPointsTrait
{
/** @var list<list{string, Server}> */
Copy link
Author

Choose a reason for hiding this comment

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

In your original PR you were storing the entire configureFailPoint command document. We only need the name of the fail point, so I've reduced this to a tuple like we originally had in FailPointObserver.

private array $failPointsAndServers = [];

public function configureFailPoint(stdClass $failPoint, Server $server): void
{
assertObjectHasAttribute('configureFailPoint', $failPoint);
assertIsString($failPoint->configureFailPoint);
assertObjectHasAttribute('mode', $failPoint);
Copy link
Author

Choose a reason for hiding this comment

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

A few assertions before we access object properties. I had these in for testing, but I suppose you can remove them if want to rely on the command succeeding.


$operation = new DatabaseCommand('admin', $failPoint);
$operation->execute($server);

if ($failPoint->mode !== 'off') {
Copy link
Author

Choose a reason for hiding this comment

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

I don't believe this is used in any tests, but it seemed reasonable to check for it since there's no point in disabling a fail point twice.

$this->failPointsAndServers[] = [$failPoint->configureFailPoint, $server];
}
}

public function disableFailPoints(): void
{
foreach ($this->failPointsAndServers as [$failPoint, $server]) {
$operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']);
$operation->execute($server);
}

$this->failPointsAndServers = [];
}
}
15 changes: 4 additions & 11 deletions tests/UnifiedSpecTests/Operation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
use MongoDB\Model\CollectionInfo;
use MongoDB\Model\DatabaseInfo;
use MongoDB\Model\IndexInfo;
use MongoDB\Operation\DatabaseCommand;
use MongoDB\Operation\FindOneAndReplace;
use MongoDB\Operation\FindOneAndUpdate;
use PHPUnit\Framework\Assert;
Expand Down Expand Up @@ -909,15 +908,17 @@ private function executeForTestRunner()
assertArrayHasKey('failPoint', $args);
assertInstanceOf(Client::class, $args['client']);
assertInstanceOf(stdClass::class, $args['failPoint']);
$this->createFailPoint($args['failPoint'], $args['client']->getManager()->selectServer());
// Configure the fail point via the Context so it can later be disabled
$this->context->configureFailPoint($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');
$this->createFailPoint($args['failPoint'], $args['session']->getServer());
// Configure the fail point via the Context so it can later be disabled
$this->context->configureFailPoint($args['failPoint'], $args['session']->getServer());
break;
case 'loop':
assertArrayHasKey('operations', $args);
Expand All @@ -936,14 +937,6 @@ 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
14 changes: 2 additions & 12 deletions tests/UnifiedSpecTests/UnifiedTestRunner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,10 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn
$operation = new Operation($o, $context);
$operation->assert();
}

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

Choose a reason for hiding this comment

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

Note that I moved stopEventObservers() and stopEventCollectors() within the finally block. That seemed correct given the following text from Executing a Test in the Unified Test Format spec:

For each element in test.operations, follow the process in Executing an Operation. If an unexpected error is encountered or an assertion fails, the test runner MUST consider this test to have failed.

If any event listeners were enabled on any client entities, the test runner MUST now disable those event listeners.

If any fail points were configured, the test runner MUST now disable those fail points (on the same server)...

It's possible that this could have resulted in memory leaks after failing tests. The subscriber would never get unregistered, and since they were registered globally they'd continue to accumulate in the process. Both EventCollector and EventObserver also hold a reference to a Context object, so those would also linger. I'm not sure if gc_collect_cycles() would have helped here, since it's PHPC itself that ultimately holds a reference to the subscriber(s) in its request-scoped HashTable.

I put this change in a separate commit, so if you think it deserves its own PHPLIB ticket for tracking feel free to create that and incorporate it into the message when merging this.

}

if (isset($test->expectEvents)) {
Expand All @@ -231,15 +230,6 @@ 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']);
$operation->execute($server);
}
}

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