Merge pull request #12254 from stanley-cheung/php-upmerge-1_4-features

PHP: upmerge from 1.4.x branch

8 files changed, 78 insertions, 55 deletions

diff --git a/package.xml b/package.xml
index 6e50badfc2..d5bd4d062a 100644
--- a/package.xml
+++ b/package.xml
@@ -22,13 +22,9 @@
  </stability>
  <license>Apache 2.0</license>
  <notes>
-- Fixed some memory leaks #9559, #10996
-- Disabled cares dependency from gRPC C Core #10940
-- De-coupled protobuf dependency #11112
-- Fixed extension reported version #10842
-- Added config.w32 for Windows support #8161
-- Fixed PHP distrib test after cc files were added #11193
-- Fixed protoc plugin comment escape bug #11025
+- Channel are now by default persistent #11878
+- Some bug fixes from 1.4 branch #12109, #12123
+- Fixed hang bug when fork() was used #11814
  </notes>
  <contents>
   <dir baseinstalldir="/" name="/">
diff --git a/src/php/ext/grpc/call_credentials.c b/src/php/ext/grpc/call_credentials.c
index f46091d709..1eee8645df 100644
--- a/src/php/ext/grpc/call_credentials.c
+++ b/src/php/ext/grpc/call_credentials.c
@@ -179,7 +179,7 @@ void plugin_get_metadata(void *ptr, grpc_auth_metadata_context context,
   grpc_metadata_array metadata;
   bool cleanup = true;
 
-  if (Z_TYPE_P(retval) != IS_ARRAY) {
+  if (retval == NULL || Z_TYPE_P(retval) != IS_ARRAY) {
     cleanup = false;
     code = GRPC_STATUS_INVALID_ARGUMENT;
   } else if (!create_metadata_array(retval, &metadata)) {
diff --git a/src/php/ext/grpc/channel.c b/src/php/ext/grpc/channel.c
index f1187e8722..dc3acc89bb 100644
--- a/src/php/ext/grpc/channel.c
+++ b/src/php/ext/grpc/channel.c
@@ -66,6 +66,8 @@ PHP_GRPC_FREE_WRAPPED_FUNC_START(wrapped_grpc_channel)
       if (!(PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list), p->wrapper->key,
                                           key_len, rsrc))) {
         grpc_channel_destroy(p->wrapper->wrapped);
+        free(p->wrapper->target);
+        free(p->wrapper->args_hashstr);
       }
       gpr_mu_unlock(&global_persistent_list_mu);
     }
@@ -158,7 +160,7 @@ void create_and_add_channel_to_persistent_list(
     grpc_channel_args args,
     wrapped_grpc_channel_credentials *creds,
     char *key,
-    php_grpc_int key_len) {
+    php_grpc_int key_len TSRMLS_DC) {
   php_grpc_zend_resource new_rsrc;
   channel_persistent_le_t *le;
   // this links each persistent list entry to a destructor
@@ -187,10 +189,8 @@ void create_and_add_channel_to_persistent_list(
  * credentials.
  *
  * If the $args array contains a "force_new" key mapping to a boolean value
- * of "true", a new underlying grpc_channel will be created regardless. If
- * there are any opened channels on the same hostname, user must manually
- * call close() on those dangling channels before the end of the PHP
- * script.
+ * of "true", a new and separate underlying grpc_channel will be created
+ * and returned. This will not affect existing channels.
  *
  * @param string $target The hostname to associate with this channel
  * @param array $args_array The arguments to pass to the Channel
@@ -273,19 +273,15 @@ PHP_METHOD(Channel, __construct) {
   }
   channel->wrapper = malloc(sizeof(grpc_channel_wrapper));
   channel->wrapper->key = key;
-  channel->wrapper->target = target;
-  channel->wrapper->args_hashstr = sha1str;
+  channel->wrapper->target = strdup(target);
+  channel->wrapper->args_hashstr = strdup(sha1str);
   if (creds != NULL && creds->hashstr != NULL) {
     channel->wrapper->creds_hashstr = creds->hashstr;
   }
   gpr_mu_init(&channel->wrapper->mu);
   smart_str_free(&buf);
 
-  if (force_new) {
-    php_grpc_delete_persistent_list_entry(key, key_len TSRMLS_CC);
-  }
-
-  if (creds != NULL && creds->has_call_creds) {
+  if (force_new || (creds != NULL && creds->has_call_creds)) {
     // If the ChannelCredentials object was composed with a CallCredentials
     // object, there is no way we can tell them apart. Do NOT persist
     // them. They should be individually destroyed.
@@ -293,7 +289,7 @@ PHP_METHOD(Channel, __construct) {
   } else if (!(PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list), key,
                                              key_len, rsrc))) {
     create_and_add_channel_to_persistent_list(
-        channel, target, args, creds, key, key_len);
+        channel, target, args, creds, key, key_len TSRMLS_CC);
   } else {
     // Found a previously stored channel in the persistent list
     channel_persistent_le_t *le = (channel_persistent_le_t *)rsrc->ptr;
@@ -303,7 +299,7 @@ PHP_METHOD(Channel, __construct) {
          strcmp(creds->hashstr, le->channel->creds_hashstr) != 0)) {
       // somehow hash collision
       create_and_add_channel_to_persistent_list(
-          channel, target, args, creds, key, key_len);
+          channel, target, args, creds, key, key_len TSRMLS_CC);
     } else {
       channel->wrapper = le->channel;
     }
@@ -416,12 +412,14 @@ PHP_METHOD(Channel, close) {
   gpr_mu_lock(&channel->wrapper->mu);
   if (channel->wrapper->wrapped != NULL) {
     grpc_channel_destroy(channel->wrapper->wrapped);
+    free(channel->wrapper->target);
+    free(channel->wrapper->args_hashstr);
     channel->wrapper->wrapped = NULL;
-  }
 
-  php_grpc_delete_persistent_list_entry(channel->wrapper->key,
-                                        strlen(channel->wrapper->key)
-                                        TSRMLS_CC);
+    php_grpc_delete_persistent_list_entry(channel->wrapper->key,
+                                          strlen(channel->wrapper->key)
+                                          TSRMLS_CC);
+  }
   gpr_mu_unlock(&channel->wrapper->mu);
 }
 
@@ -449,12 +447,11 @@ static void php_grpc_channel_plink_dtor(php_grpc_zend_resource *rsrc
     gpr_mu_lock(&le->channel->mu);
     if (le->channel->wrapped != NULL) {
       grpc_channel_destroy(le->channel->wrapped);
-      free(le->channel->key);
-      free(le->channel);
+      free(le->channel->target);
+      free(le->channel->args_hashstr);
     }
     gpr_mu_unlock(&le->channel->mu);
   }
-  free(le);
 }
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_construct, 0, 0, 2)
@@ -496,6 +493,7 @@ GRPC_STARTUP_FUNCTION(channel) {
   INIT_CLASS_ENTRY(ce, "Grpc\\Channel", channel_methods);
   ce.create_object = create_wrapped_grpc_channel;
   grpc_ce_channel = zend_register_internal_class(&ce TSRMLS_CC);
+  gpr_mu_init(&global_persistent_list_mu);
   le_plink = zend_register_list_destructors_ex(
       NULL, php_grpc_channel_plink_dtor, "Persistent Channel", module_number);
   PHP_GRPC_INIT_HANDLER(wrapped_grpc_channel, channel_ce_handlers);
diff --git a/src/php/ext/grpc/channel_credentials.c b/src/php/ext/grpc/channel_credentials.c
index 19e1cefb6f..86e4f46c67 100644
--- a/src/php/ext/grpc/channel_credentials.c
+++ b/src/php/ext/grpc/channel_credentials.c
@@ -135,6 +135,8 @@ PHP_METHOD(ChannelCredentials, createSsl) {
 
   pem_key_cert_pair.private_key = pem_key_cert_pair.cert_chain = NULL;
 
+  grpc_set_ssl_roots_override_callback(get_ssl_roots_override);
+
   /* "|s!s!s!" == 3 optional nullable strings */
   if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|s!s!s!",
                             &pem_root_certs, &root_certs_length,
@@ -148,7 +150,7 @@ PHP_METHOD(ChannelCredentials, createSsl) {
   }
 
   php_grpc_int hashkey_len = root_certs_length + cert_chain_length;
-  char hashkey[hashkey_len];
+  char *hashkey = emalloc(hashkey_len);
   if (root_certs_length > 0) {
     strcpy(hashkey, pem_root_certs);
   }
@@ -164,6 +166,7 @@ PHP_METHOD(ChannelCredentials, createSsl) {
       pem_key_cert_pair.private_key == NULL ? NULL : &pem_key_cert_pair, NULL);
   zval *creds_object = grpc_php_wrap_channel_credentials(creds, hashstr, false
                                                          TSRMLS_CC);
+  efree(hashkey);
   RETURN_DESTROY_ZVAL(creds_object);
 }
 
@@ -177,6 +180,8 @@ PHP_METHOD(ChannelCredentials, createComposite) {
   zval *cred1_obj;
   zval *cred2_obj;
 
+  grpc_set_ssl_roots_override_callback(get_ssl_roots_override);
+
   /* "OO" == 2 Objects */
   if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "OO", &cred1_obj,
                             grpc_ce_channel_credentials, &cred2_obj,
@@ -245,7 +250,6 @@ void grpc_init_channel_credentials(TSRMLS_D) {
   zend_class_entry ce;
   INIT_CLASS_ENTRY(ce, "Grpc\\ChannelCredentials",
                    channel_credentials_methods);
-  grpc_set_ssl_roots_override_callback(get_ssl_roots_override);
   ce.create_object = create_wrapped_grpc_channel_credentials;
   grpc_ce_channel_credentials = zend_register_internal_class(&ce TSRMLS_CC);
   PHP_GRPC_INIT_HANDLER(wrapped_grpc_channel_credentials,
diff --git a/src/php/ext/grpc/php_grpc.c b/src/php/ext/grpc/php_grpc.c
index a96daf7d9b..4ed56de993 100644
--- a/src/php/ext/grpc/php_grpc.c
+++ b/src/php/ext/grpc/php_grpc.c
@@ -34,7 +34,8 @@
 #include <ext/standard/info.h>
 #include "php_grpc.h"
 
-// ZEND_DECLARE_MODULE_GLOBALS(grpc)
+ZEND_DECLARE_MODULE_GLOBALS(grpc)
+static PHP_GINIT_FUNCTION(grpc);
 
 /* {{{ grpc_functions[]
  *
@@ -55,13 +56,17 @@ zend_module_entry grpc_module_entry = {
   grpc_functions,
   PHP_MINIT(grpc),
   PHP_MSHUTDOWN(grpc),
-  NULL,
+  PHP_RINIT(grpc),
   NULL,
   PHP_MINFO(grpc),
 #if ZEND_MODULE_API_NO >= 20010901
   PHP_GRPC_VERSION,
 #endif
-  STANDARD_MODULE_PROPERTIES};
+  PHP_MODULE_GLOBALS(grpc),
+  PHP_GINIT(grpc),
+  NULL,
+  NULL,
+  STANDARD_MODULE_PROPERTIES_EX};
 /* }}} */
 
 #ifdef COMPILE_DL_GRPC
@@ -99,7 +104,6 @@ PHP_MINIT_FUNCTION(grpc) {
      REGISTER_INI_ENTRIES();
   */
   /* Register call error constants */
-  grpc_init();
   REGISTER_LONG_CONSTANT("Grpc\\CALL_OK", GRPC_CALL_OK,
                          CONST_CS | CONST_PERSISTENT);
   REGISTER_LONG_CONSTANT("Grpc\\CALL_ERROR", GRPC_CALL_ERROR,
@@ -227,7 +231,6 @@ PHP_MINIT_FUNCTION(grpc) {
   grpc_init_channel_credentials(TSRMLS_C);
   grpc_init_call_credentials(TSRMLS_C);
   grpc_init_server_credentials(TSRMLS_C);
-  grpc_php_init_completion_queue(TSRMLS_C);
   return SUCCESS;
 }
 /* }}} */
@@ -240,9 +243,12 @@ PHP_MSHUTDOWN_FUNCTION(grpc) {
   */
   // WARNING: This function IS being called by PHP when the extension
   // is unloaded but the logs were somehow suppressed.
-  grpc_shutdown_timeval(TSRMLS_C);
-  grpc_php_shutdown_completion_queue(TSRMLS_C);
-  grpc_shutdown();
+  if (GRPC_G(initialized)) {
+    grpc_shutdown_timeval(TSRMLS_C);
+    grpc_php_shutdown_completion_queue(TSRMLS_C);
+    grpc_shutdown();
+    GRPC_G(initialized) = 0;
+  }
   return SUCCESS;
 }
 /* }}} */
@@ -259,6 +265,26 @@ PHP_MINFO_FUNCTION(grpc) {
   */
 }
 /* }}} */
+
+/* {{{ PHP_RINIT_FUNCTION
+ */
+PHP_RINIT_FUNCTION(grpc) {
+  if (!GRPC_G(initialized)) {
+    grpc_init();
+    grpc_php_init_completion_queue(TSRMLS_C);
+    GRPC_G(initialized) = 1;
+  }
+  return SUCCESS;
+}
+/* }}} */
+
+/* {{{ PHP_GINIT_FUNCTION
+ */
+static PHP_GINIT_FUNCTION(grpc) {
+  grpc_globals->initialized = 0;
+}
+/* }}} */
+
 /* The previous line is meant for vim and emacs, so it can correctly fold and
    unfold functions in source code. See the corresponding marks just before
    function definition, where the functions purpose is also documented. Please
diff --git a/src/php/ext/grpc/php_grpc.h b/src/php/ext/grpc/php_grpc.h
index 32329178dc..e30f011c39 100644
--- a/src/php/ext/grpc/php_grpc.h
+++ b/src/php/ext/grpc/php_grpc.h
@@ -49,14 +49,16 @@ PHP_MINIT_FUNCTION(grpc);
 PHP_MSHUTDOWN_FUNCTION(grpc);
 /* Displays information about the module */
 PHP_MINFO_FUNCTION(grpc);
+/* Code that runs at request start */
+PHP_RINIT_FUNCTION(grpc);
 
 /*
   Declare any global variables you may need between the BEGIN
   and END macros here:
-
+*/
 ZEND_BEGIN_MODULE_GLOBALS(grpc)
+  zend_bool initialized;
 ZEND_END_MODULE_GLOBALS(grpc)
-*/
 
 /* In every utility function you add that needs to use variables
    in php_grpc_globals, call TSRMLS_FETCH(); after declaring other
diff --git a/src/php/tests/unit_tests/ChannelTest.php b/src/php/tests/unit_tests/ChannelTest.php
index 400df0fb66..c375a16269 100644
--- a/src/php/tests/unit_tests/ChannelTest.php
+++ b/src/php/tests/unit_tests/ChannelTest.php
@@ -517,8 +517,6 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $state = $this->channel2->getConnectivityState();
         $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
-        // any dangling old connection to the same host must be
-        // manually closed
         $this->channel1->close();
         $this->channel2->close();
     }
@@ -529,6 +527,7 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $this->channel1 = new Grpc\Channel('localhost:1', []);
         $this->channel2 = new Grpc\Channel('localhost:1',
                                            ["force_new" => true]);
+        // channel3 shares with channel1
         $this->channel3 = new Grpc\Channel('localhost:1', []);
 
         // try to connect on channel2
@@ -540,29 +539,31 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $state = $this->channel2->getConnectivityState();
         $this->assertConnecting($state);
         $state = $this->channel3->getConnectivityState();
-        $this->assertConnecting($state);
+        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
         $this->channel1->close();
         $this->channel2->close();
     }
 
+    /**
+     * @expectedException RuntimeException
+     */
     public function testPersistentChannelForceNewOldChannelClose()
     {
 
         $this->channel1 = new Grpc\Channel('localhost:1', []);
         $this->channel2 = new Grpc\Channel('localhost:1',
                                            ["force_new" => true]);
+        // channel3 shares with channel1
         $this->channel3 = new Grpc\Channel('localhost:1', []);
 
         $this->channel1->close();
 
         $state = $this->channel2->getConnectivityState();
         $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
-        $state = $this->channel3->getConnectivityState();
-        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
-        $this->channel2->close();
-        $this->channel3->close();
+        // channel3 already closed
+        $state = $this->channel3->getConnectivityState();
     }
 
     public function testPersistentChannelForceNewNewChannelClose()
diff --git a/templates/package.xml.template b/templates/package.xml.template
index 27cc4d01a9..15da704a47 100644
--- a/templates/package.xml.template
+++ b/templates/package.xml.template
@@ -24,13 +24,9 @@
    </stability>
    <license>Apache 2.0</license>
    <notes>
-  - Fixed some memory leaks #9559, #10996
-  - Disabled cares dependency from gRPC C Core #10940
-  - De-coupled protobuf dependency #11112
-  - Fixed extension reported version #10842
-  - Added config.w32 for Windows support #8161
-  - Fixed PHP distrib test after cc files were added #11193
-  - Fixed protoc plugin comment escape bug #11025
+  - Channel are now by default persistent #11878
+  - Some bug fixes from 1.4 branch #12109, #12123
+  - Fixed hang bug when fork() was used #11814
    </notes>
    <contents>
     <dir baseinstalldir="/" name="/">