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

PHP: upmerge from 1.4.x branch

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="/">

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)) {

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);

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,

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

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

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()

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="/">