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 <[email protected]>

Author: alcaeus

tests/UnifiedSpecTests/Context.php

@@ -38,6 +38,8 @@
  */
 final class Context
 {
+    use ManagesFailPointsTrait;
+
     private ?string $activeClient = null;
 
     private EntityMap $entityMap;

tests/UnifiedSpecTests/FailPointObserver.php

@@ -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}> */
+    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 = [];
+    }
+}

tests/UnifiedSpecTests/ManagesFailPointsTrait.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,16 +908,17 @@ 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);
                 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());
+                // 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);

tests/UnifiedSpecTests/Operation.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);