PHPC-2023 and PHPC-2030: Allow observation of events dispatched during client destruction

php_phongo.c

@@ -3352,6 +3352,9 @@ void php_phongo_client_reset_once(php_phongo_manager_t* manager, int pid)
 	{
 		if (pclient->client == manager->client) {
 			phongo_pclient_reset_once(pclient, pid);
+
+			/* Request-scoped clients are only used by a single Manager, so we
+			 * can return early after finding a match. */
 			return;
 		}
 	}
@@ -3369,16 +3372,12 @@ static void php_phongo_pclient_destroy(php_phongo_pclient_t* pclient)
 	 * free_object handler or RSHUTDOWN, but there the application is capable of
 	 * freeing its Manager and its client before forking. */
 	if (pclient->created_by_pid == getpid()) {
-		/* Single-threaded clients may run commands (e.g. endSessions) from
-		 * mongoc_client_destroy, so disable APM to ensure an event is not
-		 * dispatched while destroying the Manager and its client. This means
-		 * that certain shutdown commands cannot be observed unless APM is
-		 * redesigned to not reference a client (see: PHPC-1666).
-		 *
-		 * Note: this is only relevant for request-scoped clients. APM
-		 * subscribers will no longer exist when persistent clients are
-		 * destroyed in GSHUTDOWN. */
-		mongoc_client_set_apm_callbacks(pclient->client, NULL, NULL);
+		/* If we are in request shutdown, disable APM to avoid dispatching more
+		 * events. This means that certain events (e.g. TopologyClosedEvent,
+		 * command monitoring for endSessions) may not be observed. */
+		if (EG(flags) & EG_FLAGS_IN_SHUTDOWN) {
+			mongoc_client_set_apm_callbacks(pclient->client, NULL, NULL);
+		}
 		mongoc_client_destroy(pclient->client);
 	}
 

src/MongoDB/Manager.c

@@ -918,11 +918,6 @@ static void php_phongo_manager_free_object(zend_object* object) /* {{{ */
 
 	zend_object_std_dtor(&intern->std);
 
-	/* Update the request-scoped Manager registry. The return value is ignored
-	 * because it's possible that the Manager was never registered due to a
-	 * constructor exception. */
-	php_phongo_manager_unregister(intern);
-
 	if (intern->client) {
 		/* Request-scoped clients will be removed from the registry and
 		 * destroyed. This is a NOP for persistent clients. The return value is
@@ -932,6 +927,15 @@ static void php_phongo_manager_free_object(zend_object* object) /* {{{ */
 		php_phongo_client_unregister(intern);
 	}
 
+	/* Update the request-scoped Manager registry. The return value is ignored
+	 * because it's possible that the Manager was never registered due to a
+	 * constructor exception.
+	 *
+	 * Note: this is done after unregistering a request-scoped client to ensure
+	 * APM events can be observed by per-client subscribers, which are collected
+	 * in phongo_apm_get_subscribers_to_notify. */
+	php_phongo_manager_unregister(intern);
+
 	if (intern->client_hash) {
 		efree(intern->client_hash);
 	}

tests/apm/commandStartedEvent-002.phpt

@@ -0,0 +1,42 @@
+--TEST--
+MongoDB\Driver\Monitoring\CommandStartedEvent during mongoc_client_destroy()
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
+<?php skip_if_not_libmongoc_crypto(); ?>
+<?php skip_if_not_live(); ?>
+<?php skip_if_server_version('<', '3.6'); ?>
+--FILE--
+<?php
+require_once __DIR__ . "/../utils/basic.inc";
+
+class MySubscriber implements MongoDB\Driver\Monitoring\CommandSubscriber
+{ 
+    public function commandStarted(MongoDB\Driver\Monitoring\CommandStartedEvent $event)
+    {
+        printf("Observed commandStarted for %s\n", $event->getCommandName());
+    }
+
+    public function commandSucceeded(MongoDB\Driver\Monitoring\CommandSucceededEvent $event) {}
+
+    public function commandFailed(MongoDB\Driver\Monitoring\CommandFailedEvent $event) {}
+}
+
+$manager = create_test_manager(URI, [], ['disableClientPersistence' => true]);
+
+$singleSubscriber = new MySubscriber();
+$manager->addSubscriber($singleSubscriber);
+
+$command = new MongoDB\Driver\Command(['ping' => 1]);
+$manager->executeCommand(DATABASE_NAME, $command);
+
+/* Events dispatched during mongoc_client_destroy can only be observed before
+ * RSHUTDOWN. This means that we must use a non-persistent client and free it
+ * before the script ends. */
+unset($manager);
+
+?>
+===DONE===
+--EXPECT--
+Observed commandStarted for ping
+Observed commandStarted for endSessions
+===DONE===

tests/apm/serverClosedEvent-001.phpt

@@ -1,9 +1,8 @@
 --TEST--
 MongoDB\Driver\Monitoring\ServerClosedEvent
 --SKIPIF--
-<?php echo "skip ServerClosedEvent may not be reliably observed"; /* TODO: PHPC-2023 */ ?>
 <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
-<?php skip_if_not_live(); ?>
+<?php skip_if_not_load_balanced(); ?>
 --FILE--
 <?php
 require_once __DIR__ . "/../utils/basic.inc";
@@ -45,21 +44,18 @@ class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
     public function topologyOpening(MongoDB\Driver\Monitoring\TopologyOpeningEvent $event) {}
 }
 
-/* Note: using a global subscriber works around the issue of Manager and client
- * unregistration in php_phongo_manager_free_object, but ServerClosedEvent
- * may still not be observed due to clearing APM callbacks before
- * mongoc_client_destroy in php_phongo_pclient_destroy (see: PHPC-2023) */
-MongoDB\Driver\Monitoring\addSubscriber(new MySubscriber);
-
-/* Note: ServerClosedEvent may be observed if the host in the URI does not
- * match what is reported in the hello response; however, using a non-persistent
- * client is more reliable since the event can be observed when the Manager and
- * client are freed. */
+/* Note: load balanced topologies will always emit ServerClosedEvent before
+ * TopologyClosedEvent. That is easier than adding a non-member to a replica set
+ * URI. A non-persistent client is used to make observation possible. */
 $m = create_test_manager(URI, [], ['disableClientPersistence' => true]);
+$m->addSubscriber(new MySubscriber);
 
 $command = new MongoDB\Driver\Command(['ping' => 1]);
 $m->executeCommand(DATABASE_NAME, $command);
 
+/* Events dispatched during mongoc_client_destroy can only be observed before
+ * RSHUTDOWN. This means that we must use a non-persistent client and free it
+ * before the script ends. */
 unset($m);
 
 ?>

tests/apm/topologyClosedEvent-001.phpt

@@ -1,7 +1,6 @@
 --TEST--
 MongoDB\Driver\Monitoring\TopologyClosedEvent
 --SKIPIF--
-<?php echo "skip TopologyClosedEvent cannot be observed"; /* TODO: PHPC-2023 */ ?>
 <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
 <?php skip_if_not_live(); ?>
 --FILE--
@@ -34,20 +33,18 @@ class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
     public function topologyOpening(MongoDB\Driver\Monitoring\TopologyOpeningEvent $event) {}
 }
 
-/* Note: using a global subscriber works around the issue of Manager and client
- * unregistration in php_phongo_manager_free_object, but TopologyClosedEvent
- * still cannot be observed due to clearing APM callbacks before
- * mongoc_client_destroy in php_phongo_pclient_destroy (see: PHPC-2023) */
-MongoDB\Driver\Monitoring\addSubscriber(new MySubscriber);
-
 /* Note: TopologyChangedEvent can only be observed for non-persistent clients.
  * Persistent clients are destroyed in GSHUTDOWN, long after any PHP objects
  * (including subscribers) are freed. */
 $m = create_test_manager(URI, [], ['disableClientPersistence' => true]);
+$m->addSubscriber(new MySubscriber);
 
 $command = new MongoDB\Driver\Command(['ping' => 1]);
 $m->executeCommand(DATABASE_NAME, $command);
 
+/* Events dispatched during mongoc_client_destroy can only be observed before
+ * RSHUTDOWN. This means that we must use a non-persistent client and free it
+ * before the script ends. */
 unset($m);
 
 ?>

tests/server/server-getLatency-002.phpt

@@ -2,7 +2,6 @@
 MongoDB\Driver\Server::getLatency() returns null when unset (e.g. load balancer)
 --SKIPIF--
 <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
-<?php skip_if_not_live(); ?>
 <?php skip_if_not_load_balanced(); ?>
 --FILE--
 <?php