From 90dcf186c8664a2cdd57314122d46b16234555c2 Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Fri, 20 Sep 2024 16:30:41 +0200 Subject: [PATCH] PHPLIB-1531: Replace FailPointObserver with different logic (#1427) * Extract fail point management to a trait used by Context * Ensure event listeners are still unregistered after an operation fails This change was prompted by the addition of a finally block after executing operations. It may address a memory leak whereby listeners (and the Context they reference) would remain in PHPC's global registry until RSHUTDOWN. --------- Co-authored-by: Jeremy Mikola --- tests/UnifiedSpecTests/Context.php | 2 + tests/UnifiedSpecTests/FailPointObserver.php | 63 ------------------- .../ManagesFailPointsTrait.php | 40 ++++++++++++ tests/UnifiedSpecTests/Operation.php | 8 +-- tests/UnifiedSpecTests/UnifiedTestRunner.php | 23 +++---- 5 files changed, 55 insertions(+), 81 deletions(-) delete mode 100644 tests/UnifiedSpecTests/FailPointObserver.php create mode 100644 tests/UnifiedSpecTests/ManagesFailPointsTrait.php diff --git a/tests/UnifiedSpecTests/Context.php b/tests/UnifiedSpecTests/Context.php index 18e2c3a39..57e6e7bd2 100644 --- a/tests/UnifiedSpecTests/Context.php +++ b/tests/UnifiedSpecTests/Context.php @@ -38,6 +38,8 @@ */ final class Context { + use ManagesFailPointsTrait; + private ?string $activeClient = null; private EntityMap $entityMap; diff --git a/tests/UnifiedSpecTests/FailPointObserver.php b/tests/UnifiedSpecTests/FailPointObserver.php deleted file mode 100644 index f843d43fb..000000000 --- a/tests/UnifiedSpecTests/FailPointObserver.php +++ /dev/null @@ -1,63 +0,0 @@ -getCommand(); - - if (! isset($command->configureFailPoint)) { - return; - } - - if (isset($command->mode) && $command->mode === 'off') { - return; - } - - $this->failPointsAndServers[] = [$command->configureFailPoint, $event->getServer()]; - } - - /** @see https://php.net/manual/en/mongodb-driver-monitoring-commandsubscriber.commandsucceeded.php */ - public function commandSucceeded(CommandSucceededEvent $event): void - { - } - - public function disableFailPoints(): void - { - foreach ($this->failPointsAndServers as [$failPoint, $server]) { - $operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']); - $operation->execute($server); - } - - $this->failPointsAndServers = []; - } - - public function start(): void - { - addSubscriber($this); - } - - public function stop(): void - { - removeSubscriber($this); - } -} diff --git a/tests/UnifiedSpecTests/ManagesFailPointsTrait.php b/tests/UnifiedSpecTests/ManagesFailPointsTrait.php new file mode 100644 index 000000000..d88368fa9 --- /dev/null +++ b/tests/UnifiedSpecTests/ManagesFailPointsTrait.php @@ -0,0 +1,40 @@ + */ + private array $failPointsAndServers = []; + + public function configureFailPoint(stdClass $failPoint, Server $server): void + { + assertObjectHasAttribute('configureFailPoint', $failPoint); + assertIsString($failPoint->configureFailPoint); + assertObjectHasAttribute('mode', $failPoint); + + $operation = new DatabaseCommand('admin', $failPoint); + $operation->execute($server); + + if ($failPoint->mode !== 'off') { + $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 = []; + } +} diff --git a/tests/UnifiedSpecTests/Operation.php b/tests/UnifiedSpecTests/Operation.php index 3fdf5ea70..11da19280 100644 --- a/tests/UnifiedSpecTests/Operation.php +++ b/tests/UnifiedSpecTests/Operation.php @@ -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; @@ -909,7 +908,8 @@ private function executeForTestRunner() assertArrayHasKey('failPoint', $args); assertInstanceOf(Client::class, $args['client']); assertInstanceOf(stdClass::class, $args['failPoint']); - $args['client']->selectDatabase('admin')->command($args['failPoint']); + // 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); @@ -917,8 +917,8 @@ private function executeForTestRunner() 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()); + // 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); diff --git a/tests/UnifiedSpecTests/UnifiedTestRunner.php b/tests/UnifiedSpecTests/UnifiedTestRunner.php index 644cbcaaa..f816d8456 100644 --- a/tests/UnifiedSpecTests/UnifiedTestRunner.php +++ b/tests/UnifiedSpecTests/UnifiedTestRunner.php @@ -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) @@ -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 @@ -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. */ @@ -216,14 +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(); + } + } finally { + $context->stopEventObservers(); + $context->stopEventCollectors(); + $context->disableFailPoints(); } - $context->stopEventObservers(); - $context->stopEventCollectors(); - if (isset($test->expectEvents)) { assertIsArray($test->expectEvents); $context->assertExpectedEventsForClients($test->expectEvents);