Merge pull request #168 from stesie/multithreading

Fix pthread extension compatibility

Author: stesie

Makefile.frag

@@ -3,6 +3,11 @@ ifneq (,$(realpath $(EXTENSION_DIR)/json.so))
 PHP_TEST_SHARED_EXTENSIONS+=-d extension=$(EXTENSION_DIR)/json.so
 endif
 
+# add pthreads extension, if available
+ifneq (,$(realpath $(EXTENSION_DIR)/pthreads.so))
+PHP_TEST_SHARED_EXTENSIONS+=-d extension=$(EXTENSION_DIR)/pthreads.so
+endif
+
 testv8: all
 	$(PHP_EXECUTABLE) -n -d extension_dir=./modules -d extension=v8js.so test.php
 

README.md

@@ -24,8 +24,7 @@ Minimum requirements
 
 - PHP 5.3.3+
 
-  This embedded implementation of the V8 engine uses thread locking so it should work with ZTS enabled.
-  However, this has not been tested yet.
+  This embedded implementation of the V8 engine uses thread locking so it works with ZTS enabled.
 
 
 Compiling latest version

php_v8js_macros.h

@@ -111,14 +111,10 @@ void v8js_register_accessors(std::vector<v8js_accessor_ctx*> *accessor_list, v8:
 
 /* Module globals */
 ZEND_BEGIN_MODULE_GLOBALS(v8js)
+  // Thread-local cache whether V8 has been initialized so far
   int v8_initialized;
-#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036
-  v8::Platform *v8_platform;
-#endif
-  HashTable *extensions;
 
   /* Ini globals */
-  char *v8_flags; /* V8 command line flags */
   bool use_date; /* Generate JS Date objects instead of PHP DateTime */
   bool use_array_access; /* Convert ArrayAccess, Countable objects to array-like objects */
   bool compat_php_exceptions; /* Don't stop JS execution on PHP exception */
@@ -142,6 +138,39 @@ ZEND_EXTERN_MODULE_GLOBALS(v8js)
 # define V8JSG(v) (v8js_globals.v)
 #endif
 
+/*
+ *  Process-wide globals
+ *
+ * The zend_v8js_globals structure declared above is created once per thread
+ * (in a ZTS environment).  If a multi-threaded PHP process uses V8 there is
+ * some stuff shared among all of these threads
+ *
+ *  - whether V8 has been initialized at all
+ *  - the V8 backend platform
+ *  - loaded extensions
+ *  - V8 "command line" flags
+ *
+ * In a ZTS-enabled environment access to all of these variables must happen
+ * while holding a mutex lock.
+ */
+struct _v8js_process_globals {
+#ifdef ZTS
+	int v8_initialized;
+	std::mutex lock;
+#endif
+
+	HashTable *extensions;
+
+	/* V8 command line flags */
+	char *v8_flags;
+
+#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036
+	v8::Platform *v8_platform;
+#endif
+};
+
+extern struct _v8js_process_globals v8js_process_globals;
+
 /* Register builtin methods into passed object */
 void v8js_register_methods(v8::Handle<v8::ObjectTemplate>, v8js_ctx *c);
 

tests/pthreads_001.phpt

@@ -0,0 +1,65 @@
+--TEST--
+Test V8::executeString() : Pthreads test #1
+--SKIPIF--
+<?php
+require_once(dirname(__FILE__) . '/skipif.inc');
+if(!class_exists('Thread')) {
+    die('skip pthreads extension required');
+}
+?>
+--FILE--
+<?php
+
+class Workhorse extends Thread
+{
+    protected $val;
+
+    public function __construct($val)
+    {
+        $this->val = $val;
+    }
+
+    public function run()
+    {
+        $v8 = new V8Js();
+        $v8->val = $this->val;
+        $v8->sync_var_dump = function($value) {
+            $this->synchronized(function($thread) use ($value) {
+                    while(!$thread->readyToPrint) {
+                        $thread->wait();
+                    }
+                    var_dump($value);
+                    $thread->notify();
+                }, $this);
+        };
+
+        $v8->executeString('PHP.sync_var_dump(PHP.val);');
+    }
+}
+
+$foo = new Workhorse('foo');
+$bar = new Workhorse('bar');
+
+$foo->start();
+$bar->start();
+
+$bar->synchronized(function($thread) {
+    $thread->readyToPrint = true;
+    $thread->notify();
+    $thread->wait();
+}, $bar);
+
+$foo->synchronized(function($thread) {
+    $thread->readyToPrint = true;
+    $thread->notify();
+    $thread->wait();
+}, $foo);
+
+$foo->join();
+$bar->join();
+?>
+===EOF===
+--EXPECT--
+string(3) "bar"
+string(3) "foo"
+===EOF===

v8js.cc

@@ -29,20 +29,41 @@ extern "C" {
 #include "v8js_v8object_class.h"
 
 ZEND_DECLARE_MODULE_GLOBALS(v8js)
+struct _v8js_process_globals v8js_process_globals;
 
 /* {{{ INI Settings */
 
 static ZEND_INI_MH(v8js_OnUpdateV8Flags) /* {{{ */
 {
+	bool immutable = false;
+
+#ifdef ZTS
+	v8js_process_globals.lock.lock();
+
+	if(v8js_process_globals.v8_initialized) {
+		v8js_process_globals.lock.unlock();
+		immutable = true;
+	}
+
+	v8js_process_globals.lock.unlock();
+#else
+	immutable = V8JSG(v8_initialized);
+#endif
+
+	if(immutable) {
+		/* V8 already has been initialized -> cannot be changed anymore */
+		return FAILURE;
+	}
+
 	if (new_value) {
-		if (V8JSG(v8_flags)) {
-			free(V8JSG(v8_flags));
-			V8JSG(v8_flags) = NULL;
+		if (v8js_process_globals.v8_flags) {
+			free(v8js_process_globals.v8_flags);
+			v8js_process_globals.v8_flags = NULL;
 		}
 		if (!new_value[0]) {
 			return FAILURE;
 		}
-		V8JSG(v8_flags) = zend_strndup(new_value, new_value_length);
+		v8js_process_globals.v8_flags = zend_strndup(new_value, new_value_length);
 	}
 
 	return SUCCESS;
@@ -118,6 +139,35 @@ PHP_MINIT_FUNCTION(v8js)
 static PHP_MSHUTDOWN_FUNCTION(v8js)
 {
 	UNREGISTER_INI_ENTRIES();
+
+	bool v8_initialized;
+
+#ifdef ZTS
+	v8_initialized = v8js_process_globals.v8_initialized;
+#else
+	v8_initialized = V8JSG(v8_initialized);
+#endif
+
+	if(v8_initialized) {
+		v8::V8::Dispose();
+#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036
+		v8::V8::ShutdownPlatform();
+		// @fixme call virtual destructor somehow
+		//delete v8js_process_globals.v8_platform;
+#endif
+	}
+
+	if (v8js_process_globals.v8_flags) {
+		free(v8js_process_globals.v8_flags);
+		v8js_process_globals.v8_flags = NULL;
+	}
+
+	if (v8js_process_globals.extensions) {
+		zend_hash_destroy(v8js_process_globals.extensions);
+		free(v8js_process_globals.extensions);
+		v8js_process_globals.extensions = NULL;
+	}
+
 	return SUCCESS;
 }
 /* }}} */
@@ -169,9 +219,7 @@ static PHP_GINIT_FUNCTION(v8js)
 	  run the destructors manually.
 	*/
 #ifdef ZTS
-	v8js_globals->extensions = NULL;
 	v8js_globals->v8_initialized = 0;
-	v8js_globals->v8_flags = NULL;
 
 	v8js_globals->timer_thread = NULL;
 	v8js_globals->timer_stop = false;
@@ -187,29 +235,10 @@ static PHP_GINIT_FUNCTION(v8js)
  */
 static PHP_GSHUTDOWN_FUNCTION(v8js)
 {
-	if (v8js_globals->extensions) {
-		zend_hash_destroy(v8js_globals->extensions);
-		free(v8js_globals->extensions);
-		v8js_globals->extensions = NULL;
-	}
-
-	if (v8js_globals->v8_flags) {
-		free(v8js_globals->v8_flags);
-		v8js_globals->v8_flags = NULL;
-	}
-
 #ifdef ZTS
 	v8js_globals->timer_stack.~deque();
 	v8js_globals->timer_mutex.~mutex();
 #endif
-
-	if (v8js_globals->v8_initialized) {
-		v8::V8::Dispose();
-#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036
-		v8::V8::ShutdownPlatform();
-		delete v8js_globals->v8_platform;
-#endif
-	}
 }
 /* }}} */
 

v8js_class.cc

@@ -827,10 +827,17 @@ static int v8js_register_extension(char *name, uint name_len, char *source, uint
 {
 	v8js_jsext *jsext = NULL;
 
-	if (!V8JSG(extensions)) {
-		V8JSG(extensions) = (HashTable *) malloc(sizeof(HashTable));
-		zend_hash_init(V8JSG(extensions), 1, NULL, (dtor_func_t) v8js_jsext_dtor, 1);
-	} else if (zend_hash_exists(V8JSG(extensions), name, name_len + 1)) {
+#ifdef ZTS
+	v8js_process_globals.lock.lock();
+#endif
+
+	if (!v8js_process_globals.extensions) {
+		v8js_process_globals.extensions = (HashTable *) malloc(sizeof(HashTable));
+		zend_hash_init(v8js_process_globals.extensions, 1, NULL, (dtor_func_t) v8js_jsext_dtor, 1);
+	} else if (zend_hash_exists(v8js_process_globals.extensions, name, name_len + 1)) {
+#ifdef ZTS
+		v8js_process_globals.lock.unlock();
+#endif
 		return FAILURE;
 	}
 
@@ -843,6 +850,9 @@ static int v8js_register_extension(char *name, uint name_len, char *source, uint
 			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Invalid dependency array passed");
 			v8js_jsext_dtor(jsext);
 			free(jsext);
+#ifdef ZTS
+			v8js_process_globals.lock.unlock();
+#endif
 			return FAILURE;
 		}
 	}
@@ -859,17 +869,23 @@ static int v8js_register_extension(char *name, uint name_len, char *source, uint
 
 	jsext->extension = new v8::Extension(jsext->name, jsext->source, jsext->deps_count, jsext->deps);
 
-	if (zend_hash_add(V8JSG(extensions), name, name_len + 1, jsext, sizeof(v8js_jsext), NULL) == FAILURE) {
+	if (zend_hash_add(v8js_process_globals.extensions, name, name_len + 1, jsext, sizeof(v8js_jsext), NULL) == FAILURE) {
 		v8js_jsext_dtor(jsext);
 		free(jsext);
+#ifdef ZTS
+		v8js_process_globals.lock.unlock();
+#endif
 		return FAILURE;
 	}
 
+#ifdef ZTS
+	v8js_process_globals.lock.unlock();
+#endif
+
 	jsext->extension->set_auto_enable(auto_enable ? true : false);
 	v8::RegisterExtension(jsext->extension);
 
 	free(jsext);
-
 	return SUCCESS;
 }
 /* }}} */
@@ -916,10 +932,15 @@ static PHP_METHOD(V8Js, getExtensions)
 	}
 
 	array_init(return_value);
-	if (V8JSG(extensions)) {
-		zend_hash_internal_pointer_reset_ex(V8JSG(extensions), &pos);
-		while (zend_hash_get_current_data_ex(V8JSG(extensions), (void **) &jsext, &pos) == SUCCESS) {
-			if (zend_hash_get_current_key_ex(V8JSG(extensions), &key, &key_len, &index, 0, &pos) == HASH_KEY_IS_STRING) {
+
+#ifdef ZTS
+	v8js_process_globals.lock.lock();
+#endif
+
+	if (v8js_process_globals.extensions) {
+		zend_hash_internal_pointer_reset_ex(v8js_process_globals.extensions, &pos);
+		while (zend_hash_get_current_data_ex(v8js_process_globals.extensions, (void **) &jsext, &pos) == SUCCESS) {
+			if (zend_hash_get_current_key_ex(v8js_process_globals.extensions, &key, &key_len, &index, 0, &pos) == HASH_KEY_IS_STRING) {
 				MAKE_STD_ZVAL(ext)
 				array_init(ext);
 				add_assoc_bool_ex(ext, ZEND_STRS("auto_enable"), jsext->auto_enable);
@@ -931,9 +952,13 @@ static PHP_METHOD(V8Js, getExtensions)
 				}
 				add_assoc_zval_ex(return_value, key, key_len, ext);
 			}
-			zend_hash_move_forward_ex(V8JSG(extensions), &pos);
+			zend_hash_move_forward_ex(v8js_process_globals.extensions, &pos);
 		}
 	}
+
+#ifdef ZTS
+	v8js_process_globals.lock.unlock();
+#endif
 }
 /* }}} */