PHPC-1737: Use zend_hash_graceful_reverse_destroy for persistent client HashTable (#1191)

Also improve comments for request-scoped HashTables

Author: jmikola

php_phongo.c

@@ -3722,23 +3722,29 @@ static void php_phongo_pclient_destroy_ptr(zval* ptr)
 PHP_RINIT_FUNCTION(mongodb)
 {
 	/* Initialize HashTable for non-persistent clients, which is initialized to
-	 * NULL in GINIT and destroyed and reset to NULL in RSHUTDOWN. */
+	 * NULL in GINIT and destroyed and reset to NULL in RSHUTDOWN. Although we
+	 * specify an element destructor here, all request clients should be freed
+	 * naturally via garbage collection (i.e. the HashTable should be empty at
+	 * the time it is destroyed in RSHUTDOWN). */
 	if (MONGODB_G(request_clients) == NULL) {
 		ALLOC_HASHTABLE(MONGODB_G(request_clients));
 		zend_hash_init(MONGODB_G(request_clients), 0, NULL, php_phongo_pclient_destroy_ptr, 0);
 	}
 
 	/* Initialize HashTable for APM subscribers, which is initialized to NULL in
-	 * GINIT and destroyed and reset to NULL in RSHUTDOWN. */
+	 * GINIT and destroyed and reset to NULL in RSHUTDOWN. Since this HashTable
+	 * will store subscriber object zvals, we specify ZVAL_PTR_DTOR as its
+	 * element destructor so that any still-registered subscribers can be freed
+	 * in RSHUTDOWN. */
 	if (MONGODB_G(subscribers) == NULL) {
 		ALLOC_HASHTABLE(MONGODB_G(subscribers));
 		zend_hash_init(MONGODB_G(subscribers), 0, NULL, ZVAL_PTR_DTOR, 0);
 	}
 
 	/* Initialize HashTable for registering Manager objects. This is initialized
 	 * to NULL in GINIT and destroyed and reset to NULL in RSHUTDOWN. Since this
-	 * HashTable stores pointers to existing php_phongo_manager_t objects, the
-	 * element destructor is intentionally NULL. */
+	 * HashTable stores pointers to existing php_phongo_manager_t objects (not
+	 * counted references), the element destructor is intentionally NULL. */
 	if (MONGODB_G(managers) == NULL) {
 		ALLOC_HASHTABLE(MONGODB_G(managers));
 		zend_hash_init(MONGODB_G(managers), 0, NULL, NULL, 0);
@@ -3761,8 +3767,12 @@ PHP_GINIT_FUNCTION(mongodb)
 	/* Clear extension globals */
 	memset(mongodb_globals, 0, sizeof(zend_mongodb_globals));
 
-	/* Initialize HashTable for persistent clients */
-	zend_hash_init(&mongodb_globals->persistent_clients, 0, NULL, NULL, 1);
+	/* Initialize HashTable for persistent clients, which will be destroyed in
+	 * GSHUTDOWN. We specify an element destructor so that persistent clients
+	 * can be destroyed along with the HashTable. The HashTable's struct is
+	 * nested within globals, so no allocation is needed (unlike the HashTables
+	 * allocated in RINIT). */
+	zend_hash_init(&mongodb_globals->persistent_clients, 0, NULL, php_phongo_pclient_destroy_ptr, 1);
 }
 /* }}} */
 
@@ -3934,9 +3944,11 @@ PHP_RSHUTDOWN_FUNCTION(mongodb)
 
 	/* Destroy HashTable for non-persistent clients, which was initialized in
 	 * RINIT. This is intentionally done after the APM subscribers to allow any
-	 * non-persistent clients still referenced by a subscriber (not removed
-	 * prior to RSHUTDOWN) to be naturally freed by the Manager's free_object
-	 * handler rather than the HashTable's element destructor. */
+	 * non-persistent clients still referenced by a subscriber (not freed prior
+	 * to RSHUTDOWN) to be naturally garbage collected and freed by the Manager
+	 * free_object handler rather than the HashTable's element destructor. There
+	 * is no need to use zend_hash_graceful_reverse_destroy here like we do for
+	 * persistent clients; moreover, the HashTable should already be empty. */
 	if (MONGODB_G(request_clients)) {
 		zend_hash_destroy(MONGODB_G(request_clients));
 		FREE_HASHTABLE(MONGODB_G(request_clients));
@@ -3957,20 +3969,10 @@ PHP_RSHUTDOWN_FUNCTION(mongodb)
 /* {{{ PHP_GSHUTDOWN_FUNCTION */
 PHP_GSHUTDOWN_FUNCTION(mongodb)
 {
-	php_phongo_pclient_t* pclient;
-
-	/* Destroy mongoc_client_t objects in reverse order. This is necessary to
-	 * prevent segmentation faults as clients may reference other clients in
+	/* Destroy persistent client HashTable in reverse order. This is necessary
+	 * to prevent segmentation faults as clients may reference other clients in
 	 * encryption settings. */
-	ZEND_HASH_REVERSE_FOREACH_PTR(&mongodb_globals->persistent_clients, pclient)
-	{
-		php_phongo_pclient_destroy(pclient);
-	}
-	ZEND_HASH_FOREACH_END();
-
-	/* Destroy HashTable for persistent clients. mongoc_client_t objects have
-	 * already been destroyed. */
-	zend_hash_destroy(&mongodb_globals->persistent_clients);
+	zend_hash_graceful_reverse_destroy(&mongodb_globals->persistent_clients);
 
 	mongodb_globals->debug = NULL;
 	if (mongodb_globals->debug_fd) {