diff --git a/php_phongo.c b/php_phongo.c index 1a2fa3474..c13ab9ea5 100644 --- a/php_phongo.c +++ b/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); } diff --git a/src/MongoDB/Manager.c b/src/MongoDB/Manager.c index 17db3e1d7..3761094c9 100644 --- a/src/MongoDB/Manager.c +++ b/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); } diff --git a/tests/apm/commandStartedEvent-002.phpt b/tests/apm/commandStartedEvent-002.phpt new file mode 100644 index 000000000..99ba4727c --- /dev/null +++ b/tests/apm/commandStartedEvent-002.phpt @@ -0,0 +1,42 @@ +--TEST-- +MongoDB\Driver\Monitoring\CommandStartedEvent during mongoc_client_destroy() +--SKIPIF-- + + + + +--FILE-- +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=== diff --git a/tests/apm/serverClosedEvent-001.phpt b/tests/apm/serverClosedEvent-001.phpt index 2375a6cc6..b2351af19 100644 --- a/tests/apm/serverClosedEvent-001.phpt +++ b/tests/apm/serverClosedEvent-001.phpt @@ -1,9 +1,8 @@ --TEST-- MongoDB\Driver\Monitoring\ServerClosedEvent --SKIPIF-- - - + --FILE-- 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); ?> diff --git a/tests/apm/topologyClosedEvent-001.phpt b/tests/apm/topologyClosedEvent-001.phpt index c44359aa3..cd0ec992f 100644 --- a/tests/apm/topologyClosedEvent-001.phpt +++ b/tests/apm/topologyClosedEvent-001.phpt @@ -1,7 +1,6 @@ --TEST-- MongoDB\Driver\Monitoring\TopologyClosedEvent --SKIPIF-- - --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); ?> diff --git a/tests/server/server-getLatency-002.phpt b/tests/server/server-getLatency-002.phpt index c01186341..5530c0b37 100644 --- a/tests/server/server-getLatency-002.phpt +++ b/tests/server/server-getLatency-002.phpt @@ -2,7 +2,6 @@ MongoDB\Driver\Server::getLatency() returns null when unset (e.g. load balancer) --SKIPIF-- - --FILE--