Skip to content

Commit c2410ab

Browse files
jmikolajmikola
authored
PHPC-2023 and PHPC-2030: Allow observation of events dispatched during client destruction (#1292)
* PHPC-2023: Allow observation of TopologyClosedEvent Observation is only possible for non-persistent clients freed before RSHUTDOWN. This change also allows for reliable testing of ServerClosedEvent. * PHPC-2030: Test observation of commands issued during client destruction Command monitoring events can only be observed for non-persistent clients freed before RSHUTDOWN.
1 parent 7d26cfc commit c2410ab

File tree

6 files changed

+72
-35
lines changed

6 files changed

+72
-35
lines changed

‎php_phongo.c

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3352,6 +3352,9 @@ void php_phongo_client_reset_once(php_phongo_manager_t* manager, int pid)
33523352
{
33533353
if (pclient->client == manager->client) {
33543354
phongo_pclient_reset_once(pclient, pid);
3355+
3356+
/* Request-scoped clients are only used by a single Manager, so we
3357+
* can return early after finding a match. */
33553358
return;
33563359
}
33573360
}
@@ -3369,16 +3372,12 @@ static void php_phongo_pclient_destroy(php_phongo_pclient_t* pclient)
33693372
* free_object handler or RSHUTDOWN, but there the application is capable of
33703373
* freeing its Manager and its client before forking. */
33713374
if (pclient->created_by_pid == getpid()) {
3372-
/* Single-threaded clients may run commands (e.g. endSessions) from
3373-
* mongoc_client_destroy, so disable APM to ensure an event is not
3374-
* dispatched while destroying the Manager and its client. This means
3375-
* that certain shutdown commands cannot be observed unless APM is
3376-
* redesigned to not reference a client (see: PHPC-1666).
3377-
*
3378-
* Note: this is only relevant for request-scoped clients. APM
3379-
* subscribers will no longer exist when persistent clients are
3380-
* destroyed in GSHUTDOWN. */
3381-
mongoc_client_set_apm_callbacks(pclient->client, NULL, NULL);
3375+
/* If we are in request shutdown, disable APM to avoid dispatching more
3376+
* events. This means that certain events (e.g. TopologyClosedEvent,
3377+
* command monitoring for endSessions) may not be observed. */
3378+
if (EG(flags) & EG_FLAGS_IN_SHUTDOWN) {
3379+
mongoc_client_set_apm_callbacks(pclient->client, NULL, NULL);
3380+
}
33823381
mongoc_client_destroy(pclient->client);
33833382
}
33843383

‎src/MongoDB/Manager.c

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -918,11 +918,6 @@ static void php_phongo_manager_free_object(zend_object* object) /* {{{ */
918918

919919
zend_object_std_dtor(&intern->std);
920920

921-
/* Update the request-scoped Manager registry. The return value is ignored
922-
* because it's possible that the Manager was never registered due to a
923-
* constructor exception. */
924-
php_phongo_manager_unregister(intern);
925-
926921
if (intern->client) {
927922
/* Request-scoped clients will be removed from the registry and
928923
* 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) /* {{{ */
932927
php_phongo_client_unregister(intern);
933928
}
934929

930+
/* Update the request-scoped Manager registry. The return value is ignored
931+
* because it's possible that the Manager was never registered due to a
932+
* constructor exception.
933+
*
934+
* Note: this is done after unregistering a request-scoped client to ensure
935+
* APM events can be observed by per-client subscribers, which are collected
936+
* in phongo_apm_get_subscribers_to_notify. */
937+
php_phongo_manager_unregister(intern);
938+
935939
if (intern->client_hash) {
936940
efree(intern->client_hash);
937941
}

‎tests/apm/commandStartedEvent-002.phpt

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
--TEST--
2+
MongoDB\Driver\Monitoring\CommandStartedEvent during mongoc_client_destroy()
3+
--SKIPIF--
4+
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
5+
<?php skip_if_not_libmongoc_crypto(); ?>
6+
<?php skip_if_not_live(); ?>
7+
<?php skip_if_server_version('<', '3.6'); ?>
8+
--FILE--
9+
<?php
10+
require_once __DIR__ . "/../utils/basic.inc";
11+
12+
class MySubscriber implements MongoDB\Driver\Monitoring\CommandSubscriber
13+
{
14+
public function commandStarted(MongoDB\Driver\Monitoring\CommandStartedEvent $event)
15+
{
16+
printf("Observed commandStarted for %s\n", $event->getCommandName());
17+
}
18+
19+
public function commandSucceeded(MongoDB\Driver\Monitoring\CommandSucceededEvent $event) {}
20+
21+
public function commandFailed(MongoDB\Driver\Monitoring\CommandFailedEvent $event) {}
22+
}
23+
24+
$manager = create_test_manager(URI, [], ['disableClientPersistence' => true]);
25+
26+
$singleSubscriber = new MySubscriber();
27+
$manager->addSubscriber($singleSubscriber);
28+
29+
$command = new MongoDB\Driver\Command(['ping' => 1]);
30+
$manager->executeCommand(DATABASE_NAME, $command);
31+
32+
/* Events dispatched during mongoc_client_destroy can only be observed before
33+
* RSHUTDOWN. This means that we must use a non-persistent client and free it
34+
* before the script ends. */
35+
unset($manager);
36+
37+
?>
38+
===DONE===
39+
--EXPECT--
40+
Observed commandStarted for ping
41+
Observed commandStarted for endSessions
42+
===DONE===

‎tests/apm/serverClosedEvent-001.phpt

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
--TEST--
22
MongoDB\Driver\Monitoring\ServerClosedEvent
33
--SKIPIF--
4-
<?php echo "skip ServerClosedEvent may not be reliably observed"; /* TODO: PHPC-2023 */ ?>
54
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
6-
<?php skip_if_not_live(); ?>
5+
<?php skip_if_not_load_balanced(); ?>
76
--FILE--
87
<?php
98
require_once __DIR__ . "/../utils/basic.inc";
@@ -45,21 +44,18 @@ class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
4544
public function topologyOpening(MongoDB\Driver\Monitoring\TopologyOpeningEvent $event) {}
4645
}
4746

48-
/* Note: using a global subscriber works around the issue of Manager and client
49-
* unregistration in php_phongo_manager_free_object, but ServerClosedEvent
50-
* may still not be observed due to clearing APM callbacks before
51-
* mongoc_client_destroy in php_phongo_pclient_destroy (see: PHPC-2023) */
52-
MongoDB\Driver\Monitoring\addSubscriber(new MySubscriber);
53-
54-
/* Note: ServerClosedEvent may be observed if the host in the URI does not
55-
* match what is reported in the hello response; however, using a non-persistent
56-
* client is more reliable since the event can be observed when the Manager and
57-
* client are freed. */
47+
/* Note: load balanced topologies will always emit ServerClosedEvent before
48+
* TopologyClosedEvent. That is easier than adding a non-member to a replica set
49+
* URI. A non-persistent client is used to make observation possible. */
5850
$m = create_test_manager(URI, [], ['disableClientPersistence' => true]);
51+
$m->addSubscriber(new MySubscriber);
5952

6053
$command = new MongoDB\Driver\Command(['ping' => 1]);
6154
$m->executeCommand(DATABASE_NAME, $command);
6255

56+
/* Events dispatched during mongoc_client_destroy can only be observed before
57+
* RSHUTDOWN. This means that we must use a non-persistent client and free it
58+
* before the script ends. */
6359
unset($m);
6460

6561
?>

‎tests/apm/topologyClosedEvent-001.phpt

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
--TEST--
22
MongoDB\Driver\Monitoring\TopologyClosedEvent
33
--SKIPIF--
4-
<?php echo "skip TopologyClosedEvent cannot be observed"; /* TODO: PHPC-2023 */ ?>
54
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
65
<?php skip_if_not_live(); ?>
76
--FILE--
@@ -34,20 +33,18 @@ class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
3433
public function topologyOpening(MongoDB\Driver\Monitoring\TopologyOpeningEvent $event) {}
3534
}
3635

37-
/* Note: using a global subscriber works around the issue of Manager and client
38-
* unregistration in php_phongo_manager_free_object, but TopologyClosedEvent
39-
* still cannot be observed due to clearing APM callbacks before
40-
* mongoc_client_destroy in php_phongo_pclient_destroy (see: PHPC-2023) */
41-
MongoDB\Driver\Monitoring\addSubscriber(new MySubscriber);
42-
4336
/* Note: TopologyChangedEvent can only be observed for non-persistent clients.
4437
* Persistent clients are destroyed in GSHUTDOWN, long after any PHP objects
4538
* (including subscribers) are freed. */
4639
$m = create_test_manager(URI, [], ['disableClientPersistence' => true]);
40+
$m->addSubscriber(new MySubscriber);
4741

4842
$command = new MongoDB\Driver\Command(['ping' => 1]);
4943
$m->executeCommand(DATABASE_NAME, $command);
5044

45+
/* Events dispatched during mongoc_client_destroy can only be observed before
46+
* RSHUTDOWN. This means that we must use a non-persistent client and free it
47+
* before the script ends. */
5148
unset($m);
5249

5350
?>

‎tests/server/server-getLatency-002.phpt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
MongoDB\Driver\Server::getLatency() returns null when unset (e.g. load balancer)
33
--SKIPIF--
44
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
5-
<?php skip_if_not_live(); ?>
65
<?php skip_if_not_load_balanced(); ?>
76
--FILE--
87
<?php

0 commit comments

Comments
 (0)