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
diff --git a/.gitignore b/.gitignore
index 4e909ae..5e38bbf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -141,6 +141,8 @@
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
diff --git a/Makefile.am b/Makefile.am
index 8fd66c7..ed852c8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -877,6 +877,8 @@
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 \
diff --git a/php/ext/google/protobuf/protobuf.c b/php/ext/google/protobuf/protobuf.c
index fb4c209..e9ac935 100644
--- a/php/ext/google/protobuf/protobuf.c
+++ b/php/ext/google/protobuf/protobuf.c
@@ -49,8 +49,8 @@
// 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,16 +138,28 @@
}
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);
@@ -155,16 +167,26 @@
}
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 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 @@
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 @@
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);
}
diff --git a/php/tests/compile_extension.sh b/php/tests/compile_extension.sh
index 24367dc..2ffc51f 100755
--- a/php/tests/compile_extension.sh
+++ b/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'
diff --git a/php/tests/multirequest.php b/php/tests/multirequest.php
new file mode 100644
index 0000000..bbe8d77
--- /dev/null
+++ b/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>";
+}
diff --git a/php/tests/multirequest.sh b/php/tests/multirequest.sh
new file mode 100755
index 0000000..97535ea
--- /dev/null
+++ b/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
diff --git a/php/tests/test.sh b/php/tests/test.sh
index 9ef565c..3c5e30d 100755
--- a/php/tests/test.sh
+++ b/php/tests/test.sh
@@ -7,7 +7,7 @@
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)
diff --git a/tests.sh b/tests.sh
index 7605e64..65dc15f 100755
--- a/tests.sh
+++ b/tests.sh
@@ -534,7 +534,9 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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() {
build_php_all_32 true
+ build_php_multirequest
build_php_compatibility
}
