From e1e3f974ec5e3adee3743b6329bffa40c778d694 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Wed, 20 Jan 2021 14:20:08 +0800 Subject: [PATCH 1/2] PHPC-1737: Use zend_hash_graceful_reverse_destroy for persistent client HashTable --- php_phongo.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/php_phongo.c b/php_phongo.c index f30b06349..4ff9647a3 100644 --- a/php_phongo.c +++ b/php_phongo.c @@ -3761,8 +3761,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); } /* }}} */ @@ -3957,20 +3961,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) { From 1537ad0526bb7d866f70f3f0747dfb602baefb53 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Wed, 20 Jan 2021 14:20:48 +0800 Subject: [PATCH 2/2] Improve comments for request-scoped HashTables --- php_phongo.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/php_phongo.c b/php_phongo.c index 4ff9647a3..605f64895 100644 --- a/php_phongo.c +++ b/php_phongo.c @@ -3722,14 +3722,20 @@ 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); @@ -3737,8 +3743,8 @@ PHP_RINIT_FUNCTION(mongodb) /* 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); @@ -3938,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));