Fix Multiple Request for PHP (#7008)

* Add scripts to test multirequest

* chmod ug+x multirequest.sh

* Add continuous test

* Compile c extension

* Class entry is obsolete in the second request
1) Needes to use class name in persistent map
2) Invalidate class entry stored in descriptor

* Add new files to dist

* Fix compile_extension

* Cleanup outputs for phpize

Author: TeBoring

.gitignore

@@ -141,6 +141,8 @@ php/tests/old_protoc
 php/tests/protobuf/
 php/tests/core
 php/tests/vgcore*
+php/tests/multirequest.result
+php/tests/nohup.out
 php/ext/google/protobuf/.libs/
 php/ext/google/protobuf/Makefile.fragments
 php/ext/google/protobuf/Makefile.global

Makefile.am

@@ -877,6 +877,8 @@ php_EXTRA_DIST=                                                       \
   php/tests/generated_service_test.php                                \
   php/tests/map_field_test.php                                        \
   php/tests/memory_leak_test.php                                      \
+  php/tests/multirequest.php                                          \
+  php/tests/multirequest.sh                                           \
   php/tests/php_implementation_test.php                               \
   php/tests/proto/empty/echo.proto                                    \
   php/tests/proto/test.proto                                          \

php/ext/google/protobuf/protobuf.c

@@ -49,8 +49,8 @@ static upb_inttable upb_def_to_enumdesc_map_persistent;
 // Global map from message/enum's php class entry to corresponding wrapper
 // Descriptor/EnumDescriptor instances.
 static HashTable* ce_to_php_obj_map;
-static upb_inttable ce_to_desc_map_persistent;
-static upb_inttable ce_to_enumdesc_map_persistent;
+static upb_strtable ce_to_desc_map_persistent;
+static upb_strtable ce_to_enumdesc_map_persistent;
 // Global map from message/enum's proto fully-qualified name to corresponding
 // wrapper Descriptor/EnumDescriptor instances.
 static upb_strtable proto_to_desc_map_persistent;
@@ -138,33 +138,55 @@ PHP_PROTO_HASHTABLE_VALUE get_ce_obj(const void* ce) {
 }
 
 void add_ce_desc(const zend_class_entry* ce, DescriptorInternal* desc) {
-  upb_inttable_insertptr(&ce_to_desc_map_persistent,
-                         ce, upb_value_ptr(desc));
+#if PHP_MAJOR_VERSION < 7
+  const char* klass = ce->name;
+#else
+  const char* klass = ZSTR_VAL(ce->name);
+#endif
+  upb_strtable_insert(&ce_to_desc_map_persistent, klass,
+                      upb_value_ptr(desc));
 }
 
 DescriptorInternal* get_ce_desc(const zend_class_entry* ce) {
+#if PHP_MAJOR_VERSION < 7
+  const char* klass = ce->name;
+#else
+  const char* klass = ZSTR_VAL(ce->name);
+#endif
+
   upb_value v;
 #ifndef NDEBUG
   v.ctype = UPB_CTYPE_PTR;
 #endif
-  if (!upb_inttable_lookupptr(&ce_to_desc_map_persistent, ce, &v)) {
+
+  if (!upb_strtable_lookup(&ce_to_desc_map_persistent, klass, &v)) {
     return NULL;
   } else {
     return upb_value_getptr(v);
   }
 }
 
 void add_ce_enumdesc(const zend_class_entry* ce, EnumDescriptorInternal* desc) {
-  upb_inttable_insertptr(&ce_to_enumdesc_map_persistent,
-                         ce, upb_value_ptr(desc));
+#if PHP_MAJOR_VERSION < 7
+  const char* klass = ce->name;
+#else
+  const char* klass = ZSTR_VAL(ce->name);
+#endif
+  upb_strtable_insert(&ce_to_enumdesc_map_persistent, klass,
+                      upb_value_ptr(desc));
 }
 
 EnumDescriptorInternal* get_ce_enumdesc(const zend_class_entry* ce) {
+#if PHP_MAJOR_VERSION < 7
+  const char* klass = ce->name;
+#else
+  const char* klass = ZSTR_VAL(ce->name);
+#endif
   upb_value v;
 #ifndef NDEBUG
   v.ctype = UPB_CTYPE_PTR;
 #endif
-  if (!upb_inttable_lookupptr(&ce_to_enumdesc_map_persistent, ce, &v)) {
+  if (!upb_strtable_lookup(&ce_to_enumdesc_map_persistent, klass, &v)) {
     return NULL;
   } else {
     return upb_value_getptr(v);
@@ -330,8 +352,8 @@ static void php_proto_hashtable_descriptor_release(zval* value) {
 static void initialize_persistent_descriptor_pool(TSRMLS_D) {
   upb_inttable_init(&upb_def_to_desc_map_persistent, UPB_CTYPE_PTR);
   upb_inttable_init(&upb_def_to_enumdesc_map_persistent, UPB_CTYPE_PTR);
-  upb_inttable_init(&ce_to_desc_map_persistent, UPB_CTYPE_PTR);
-  upb_inttable_init(&ce_to_enumdesc_map_persistent, UPB_CTYPE_PTR);
+  upb_strtable_init(&ce_to_desc_map_persistent, UPB_CTYPE_PTR);
+  upb_strtable_init(&ce_to_enumdesc_map_persistent, UPB_CTYPE_PTR);
   upb_strtable_init(&proto_to_desc_map_persistent, UPB_CTYPE_PTR);
   upb_strtable_init(&class_to_desc_map_persistent, UPB_CTYPE_PTR);
 
@@ -360,7 +382,29 @@ static PHP_RINIT_FUNCTION(protobuf) {
   generated_pool_php = NULL;
   internal_generated_pool_php = NULL;
 
-  if (!PROTOBUF_G(keep_descriptor_pool_after_request)) {
+  if (PROTOBUF_G(keep_descriptor_pool_after_request)) {
+    // Needs to clean up obsolete class entry
+    upb_strtable_iter i;
+    upb_value v;
+
+    DescriptorInternal* desc;
+    for(upb_strtable_begin(&i, &ce_to_desc_map_persistent);
+        !upb_strtable_done(&i);
+        upb_strtable_next(&i)) {
+      v = upb_strtable_iter_value(&i);
+      desc = upb_value_getptr(v);
+      desc->klass = NULL;
+    }
+
+    EnumDescriptorInternal* enumdesc;
+    for(upb_strtable_begin(&i, &ce_to_enumdesc_map_persistent);
+        !upb_strtable_done(&i);
+        upb_strtable_next(&i)) {
+      v = upb_strtable_iter_value(&i);
+      enumdesc = upb_value_getptr(v);
+      enumdesc->klass = NULL;
+    }
+  } else {
     initialize_persistent_descriptor_pool(TSRMLS_C);
   }
 
@@ -410,8 +454,8 @@ static void cleanup_persistent_descriptor_pool(TSRMLS_D) {
 
   upb_inttable_uninit(&upb_def_to_desc_map_persistent);
   upb_inttable_uninit(&upb_def_to_enumdesc_map_persistent);
-  upb_inttable_uninit(&ce_to_desc_map_persistent);
-  upb_inttable_uninit(&ce_to_enumdesc_map_persistent);
+  upb_strtable_uninit(&ce_to_desc_map_persistent);
+  upb_strtable_uninit(&ce_to_enumdesc_map_persistent);
   upb_strtable_uninit(&proto_to_desc_map_persistent);
   upb_strtable_uninit(&class_to_desc_map_persistent);
 }

php/tests/compile_extension.sh

@@ -1,8 +1,12 @@
 #!/bin/bash
 
-EXTENSION_PATH=$1
+VERSION=$2
 
-pushd $EXTENSION_PATH
+export PATH=/usr/local/php-$VERSION/bin:$PATH
+export C_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$C_INCLUDE_PATH
+export CPLUS_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$CPLUS_INCLUDE_PATH
+
+pushd  ../ext/google/protobuf
 make clean || true
 set -e
 # Add following in configure for debug: --enable-debug CFLAGS='-g -O0'

php/tests/multirequest.php

@@ -0,0 +1,8 @@
+<?php
+
+if (extension_loaded("protobuf")) {
+    require_once('memory_leak_test.php');
+    echo "<p>protobuf loaded</p>";
+} else {
+    echo "<p>protobuf not loaded</p>";
+}

php/tests/multirequest.sh

@@ -0,0 +1,34 @@
+#!/bin/bash
+set -e
+
+# Compile c extension
+VERSION=7.4
+PORT=12345
+
+export PATH=/usr/local/php-$VERSION/bin:$PATH
+export C_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$C_INCLUDE_PATH
+export CPLUS_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$CPLUS_INCLUDE_PATH
+/bin/bash ./compile_extension.sh $VERSION
+
+nohup php -d protobuf.keep_descriptor_pool_after_request=1 -dextension=../ext/google/protobuf/modules/protobuf.so -S localhost:$PORT multirequest.php 2>&1 &
+
+sleep 1
+
+wget http://localhost:$PORT/multirequest.result -O multirequest.result
+wget http://localhost:$PORT/multirequest.result -O multirequest.result
+
+pushd ../ext/google/protobuf
+phpize --clean
+popd
+
+PID=`ps | grep "php" | awk '{print $1}'`
+echo $PID
+
+if [[ -z "$PID" ]]
+then
+  echo "Failed"
+  exit 1
+else
+  kill $PID
+  echo "Succeeded"
+fi

php/tests/test.sh

@@ -7,7 +7,7 @@ export C_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$V
 export CPLUS_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$CPLUS_INCLUDE_PATH
 
 # Compile c extension
-/bin/bash ./compile_extension.sh ../ext/google/protobuf
+/bin/bash ./compile_extension.sh $VERSION
 
 tests=( array_test.php encode_decode_test.php generated_class_test.php map_field_test.php well_known_test.php descriptors_test.php wrapper_type_setters_test.php)
 

tests.sh

@@ -534,7 +534,9 @@ build_php5.5_mixed() {
   pushd php
   rm -rf vendor
   composer update
-  /bin/bash ./tests/compile_extension.sh ./ext/google/protobuf
+  pushd tests
+  /bin/bash ./compile_extension.sh 5.5
+  popd
   php -dextension=./ext/google/protobuf/modules/protobuf.so ./vendor/bin/phpunit
   popd
 }
@@ -584,7 +586,9 @@ build_php5.6_mixed() {
   pushd php
   rm -rf vendor
   composer update
-  /bin/bash ./tests/compile_extension.sh ./ext/google/protobuf
+  pushd tests
+  /bin/bash ./compile_extension.sh 5.6
+  popd
   php -dextension=./ext/google/protobuf/modules/protobuf.so ./vendor/bin/phpunit
   popd
 }
@@ -658,7 +662,9 @@ build_php7.0_mixed() {
   pushd php
   rm -rf vendor
   composer update
-  /bin/bash ./tests/compile_extension.sh ./ext/google/protobuf
+  pushd tests
+  /bin/bash ./compile_extension.sh 7.0
+  popd
   php -dextension=./ext/google/protobuf/modules/protobuf.so ./vendor/bin/phpunit
   popd
 }
@@ -706,6 +712,13 @@ build_php_compatibility() {
   php/tests/compatibility_test.sh $LAST_RELEASED
 }
 
+build_php_multirequest() {
+  use_php 7.4
+  pushd php/tests
+  ./multirequest.sh
+  popd
+}
+
 build_php7.1() {
   use_php 7.1
   pushd php
@@ -737,7 +750,9 @@ build_php7.1_mixed() {
   pushd php
   rm -rf vendor
   composer update
-  /bin/bash ./tests/compile_extension.sh ./ext/google/protobuf
+  pushd tests
+  /bin/bash ./compile_extension.sh 7.1
+  popd
   php -dextension=./ext/google/protobuf/modules/protobuf.so ./vendor/bin/phpunit
   popd
 }
@@ -790,7 +805,9 @@ build_php7.4_mixed() {
   pushd php
   rm -rf vendor
   composer update
-  /bin/bash ./tests/compile_extension.sh ./ext/google/protobuf
+  pushd tests
+  /bin/bash ./compile_extension.sh 7.4
+  popd
   php -dextension=./ext/google/protobuf/modules/protobuf.so ./vendor/bin/phpunit
   popd
   pushd php/ext/google/protobuf
@@ -840,6 +857,7 @@ build_php_all_32() {
 
 build_php_all() {
   build_php_all_32 true
+  build_php_multirequest
   build_php_compatibility
 }