Commits

Ruslan Osmanov  committed dae692a

Dev: Fixing memory leaks

  • Participants
  • Parent commits 67548f0

Comments (0)

Files changed (10)

 /* }}} */
 
 /* {{{ php_ev_write_property */
-void php_ev_write_property(zval *object, zval *member, zval *value, const zend_literal *key TSRMLS_DC)
+static void php_ev_write_property(zval *object, zval *member, zval *value, const zend_literal *key TSRMLS_DC)
 {
 	zval                 tmp_member;
 	php_ev_object       *obj;
 /* {{{ php_ev_has_property */
 static int php_ev_has_property(zval *object, zval *member, int has_set_exists, const zend_literal *key TSRMLS_DC)
 {
-	php_ev_object   *obj = (php_ev_object *) zend_objects_get_address(object TSRMLS_CC);
-	int              ret = 0;
+	php_ev_object	*obj = (php_ev_object *) zend_objects_get_address(object TSRMLS_CC);
+	int				 ret = 0;
 	php_ev_prop_handler  p;
 
 	if (zend_hash_find(obj->prop_handler, Z_STRVAL_P(member), Z_STRLEN_P(member) + 1, (void **) &p) == SUCCESS) {
-	    switch (has_set_exists) {
-	        case 2:
-	            ret = 1;
-	            break;
-	        case 1: {
-	                	zval *value = php_ev_read_property(object, member, BP_VAR_IS, key TSRMLS_CC);
-	                	if (value != EG(uninitialized_zval_ptr)) {
-	                	    convert_to_boolean(value);
-	                	    ret = Z_BVAL_P(value)? 1:0;
-	                	    /* refcount is 0 */
-	                	    Z_ADDREF_P(value);
-	                	    zval_ptr_dtor(&value);
-	                	}
-	                	break;
-	                }
-	        case 0:{
-	                   zval *value = php_ev_read_property(object, member, BP_VAR_IS, key TSRMLS_CC);
-	                   if (value != EG(uninitialized_zval_ptr)) {
-	                	   ret = Z_TYPE_P(value) != IS_NULL? 1:0;
-	                	   /* refcount is 0 */
-	                	   Z_ADDREF_P(value);
-	                	   zval_ptr_dtor(&value);
-	                   }
-	                   break;
-	               }
-	        default:
-	               php_error_docref(NULL TSRMLS_CC, E_WARNING, "Invalid value for has_set_exists");
-	    }
+		switch (has_set_exists) {
+			case 2:
+				ret = 1;
+				break;
+			case 1: {
+						zval *value = php_ev_read_property(object, member, BP_VAR_IS, key TSRMLS_CC);
+						if (value != EG(uninitialized_zval_ptr)) {
+							convert_to_boolean(value);
+							ret = Z_BVAL_P(value)? 1:0;
+							/* refcount is 0 */
+							Z_ADDREF_P(value);
+							zval_ptr_dtor(&value);
+						}
+						break;
+					}
+			case 0:{
+					   zval *value = php_ev_read_property(object, member, BP_VAR_IS, key TSRMLS_CC);
+					   if (value != EG(uninitialized_zval_ptr)) {
+						   ret = Z_TYPE_P(value) != IS_NULL? 1:0;
+						   /* refcount is 0 */
+						   Z_ADDREF_P(value);
+						   zval_ptr_dtor(&value);
+					   }
+					   break;
+				   }
+			default:
+				   php_error_docref(NULL TSRMLS_CC, E_WARNING, "Invalid value for has_set_exists");
+		}
 	} else {
-	    zend_object_handlers *std_hnd = zend_get_std_object_handlers();
-	    ret = std_hnd->has_property(object, member, has_set_exists, key TSRMLS_CC);
+		zend_object_handlers *std_hnd = zend_get_std_object_handlers();
+		ret = std_hnd->has_property(object, member, has_set_exists, key TSRMLS_CC);
 	}
 	return ret;
 } /* }}} */
 		o_loop->w = NULL;
 	}
 
-#if 0
-	if (o_loop) {
-		ev_watcher *pw = o_loop->w;
-
-		if (pw == ptr) { /* head of the list */
-			o_loop->w = w_next;
-		} else {
-			while (pw) {
-				if (php_ev_watcher_next(pw) == ptr) {
-					/* pw is the next watcher after ptr */
-					php_ev_watcher_next(pw) = w_next;
-					break;
-				}
-				pw = php_ev_watcher_next(pw);
-			}
-		}
-	}
-#endif
-
 	php_ev_watcher_next(ptr) = php_ev_watcher_prev(ptr) = NULL;
 
 	PHP_EV_FREE_FCALL_INFO(php_ev_watcher_fci(ptr), php_ev_watcher_fcc(ptr));
 		php_ev_watcher_data(ptr) = NULL;
 	}
 
-	zval_ptr_dtor(&php_ev_watcher_self(ptr));
+	if (php_ev_watcher_self(ptr)) {
+		zval_ptr_dtor(&php_ev_watcher_self(ptr));
+		php_ev_watcher_self(ptr) = NULL;
+	}
 }
 /* }}} */
 
 {
 	php_ev_object *obj_ptr = (php_ev_object *) object;
 
-	PHP_EV_ASSERT(obj_ptr->ptr);
+	if (UNEXPECTED(!obj_ptr->ptr)) return;
 	ev_timer *ptr = (ev_timer *) obj_ptr->ptr;
 
 	/* Free base class members */
         pfcc_dst = NULL;                                                                        \
     }                                                                                           \
 
-#define PHP_EV_FREE_FCALL_INFO(pfci, pfcc)       \
-    if (pfci && pfcc) {                          \
-        efree(pfcc);                             \
-                                                 \
-        if (ZEND_FCI_INITIALIZED(*pfci)) {       \
-            PHP_EV_FCI_DELREF(pfci);             \
-        }                                        \
-        efree(pfci);                             \
-    }                                            \
+#define PHP_EV_FREE_FCALL_INFO(pfci, pfcc)        \
+    if (pfci && pfcc) {                           \
+        efree(pfcc);                              \
+        pfcc = NULL;                              \
+                                                  \
+        if (ZEND_FCI_INITIALIZED(*pfci)) {        \
+            PHP_EV_FCI_DELREF(pfci);              \
+        }                                         \
+        efree(pfci);                              \
+        pfci = NULL;                              \
+    }
 
 #define PHP_EV_LOOP_OBJECT_FETCH_FROM_OBJECT(obj) (obj ? (php_ev_loop *) obj->ptr : NULL)
 #define PHP_EV_WATCHER_FETCH_FROM_OBJECT(o)       ((ev_watcher *) o->ptr)
 #include "watcher.h"
 #include <fcntl.h>
 
+#define PHP_EV_PROP_REQUIRE(x) \
+	do {                       \
+		if (!(x)) {            \
+			return FAILURE;    \
+		}                      \
+	} while (0);
+
 static inline void php_ev_prop_write_zval(zval **ppz, zval *value)
 {
+#if 0
+	if (!*ppz) {
+		/* if we assign referenced variable, we should separate it */
+		Z_ADDREF_P(value);
+		if (PZVAL_IS_REF(value)) {
+			SEPARATE_ZVAL(&value);
+		}
+		*ppz = value;
+	} else if (PZVAL_IS_REF(*ppz)) {
+		zval garbage = **ppz; /* old value should be destroyed */
+
+		/* To check: can't *ppz be some system variable like error_zval here? */
+		Z_TYPE_PP(ppz) = Z_TYPE_P(value);
+		(*ppz)->value = value->value;
+		if (Z_REFCOUNT_P(value) > 0) {
+			zval_copy_ctor(*ppz);
+		}
+		zval_dtor(&garbage);
+	} else {
+		zval *garbage = *ppz;
+
+		/* if we assign referenced variable, we should separate it */
+		Z_ADDREF_P(value);
+		if (PZVAL_IS_REF(value)) {
+			SEPARATE_ZVAL(&value);
+		}
+		*ppz = value;
+		zval_ptr_dtor(&garbage);
+	}
+
+#else
+
 	if (!*ppz) {
 		MAKE_STD_ZVAL(*ppz);
 	}
 
-    /* Make a copy of the zval, avoid direct binding to the address
-     * of value, since it breaks refcount in php_ev_read_property()
-     * causing further leaks and memory access violations */
+	/* Make a copy of the zval, avoid direct binding to the address
+	 * of value, since it breaks refcount in read_property()
+	 * causing further leaks and memory access violations */
 	REPLACE_ZVAL_VALUE(ppz, value, PZVAL_IS_REF((zval *)value));
+
+#endif
 }
 
 static inline void php_ev_prop_read_zval(zval *pz, zval **retval)
 		return;
 	}
 
-    MAKE_STD_ZVAL(*retval);
-#if 0
-    REPLACE_ZVAL_VALUE(retval, pz, PZVAL_IS_REF((zval *)pz));
-#endif
+	MAKE_STD_ZVAL(*retval);
 	ZVAL_ZVAL(*retval, pz, 1, 0);
 }
 
 /* {{{ ev_loop_prop_data_get_ptr_ptr */
 static zval **ev_loop_prop_data_get_ptr_ptr(php_ev_object *obj TSRMLS_DC)
 {
-	PHP_EV_ASSERT(obj->ptr);
+	if (!obj->ptr) return NULL;
 
 	if (!PHP_EV_LOOP_OBJECT_FETCH_FROM_OBJECT(obj)->data) {
 		MAKE_STD_ZVAL(PHP_EV_LOOP_OBJECT_FETCH_FROM_OBJECT(obj)->data);
 /* {{{ ev_loop_prop_data_read  */
 static int ev_loop_prop_data_read(php_ev_object *obj, zval **retval TSRMLS_DC)
 {
-	PHP_EV_ASSERT(obj->ptr);
+	PHP_EV_PROP_REQUIRE(obj->ptr);
 
 	zval *data = PHP_EV_LOOP_OBJECT_FETCH_FROM_OBJECT(obj)->data;
 
 /* {{{ ev_loop_prop_data_write  */
 static int ev_loop_prop_data_write(php_ev_object *obj, zval *value TSRMLS_DC)
 {
-	PHP_EV_ASSERT(obj->ptr);
+	PHP_EV_PROP_REQUIRE(obj->ptr);
 
 	php_ev_prop_write_zval(&(PHP_EV_LOOP_OBJECT_FETCH_FROM_OBJECT(obj))->data, value);
 
 /* {{{ ev_watcher_prop_data_get_ptr_ptr */
 static zval **ev_watcher_prop_data_get_ptr_ptr(php_ev_object *obj TSRMLS_DC)
 {
-	PHP_EV_ASSERT(obj->ptr);
+	if (!obj->ptr) return NULL;
 
 	zval *data = PHP_EV_WATCHER_FETCH_FROM_OBJECT(obj)->data;
 	if (!data) {
 /* {{{ ev_watcher_prop_data_write */
 static int ev_watcher_prop_data_write(php_ev_object *obj, zval *value TSRMLS_DC)
 {
-	PHP_EV_ASSERT(obj->ptr);
+	PHP_EV_PROP_REQUIRE(obj->ptr);
 
-	php_ev_prop_write_zval(&(PHP_EV_WATCHER_FETCH_FROM_OBJECT(obj))->data, value);
+	zval **data = &(PHP_EV_WATCHER_FETCH_FROM_OBJECT(obj))->data;
+	php_ev_prop_write_zval(data, value);
 
 	return SUCCESS;
 }

File tests/05_timer.phpt

 Check for EvTimer functionality
 --FILE--
 <?php 
-error_reporting(0);
+//error_reporting(0);
 
 $fudge = 0.02;
 $id = 1;
 $base = Ev::now();
 $prev = Ev::now();
 
+$timer = array();
+$periodic = array();
+
 for ($i = 1; $i <= 5; ++$i) {
 	$t = $i * $i * 1.735435336;
 	$t -= (int) $t;
 
-	$timer = new EvTimer($t, 0, function ($w, $r)
+	$tmp_timer = new EvTimer($t, 0, function ($w, $r)
 		use (&$id, &$prev, $base, $i, $t, $fudge) {
 			$now = Ev::now();
 
 
 			$prev = $now;
 		});
-	$timer->start();
+	$tmp_timer->start();
+	$timer[] = $tmp_timer;
 
 	$t = $i * $i * 1.375475771;
 	$t -= (int) $t;
 
-	$periodic = new EvPeriodic($base + $t, 0, NULL, function ($w, $r)
+	$tmp_periodic = new EvPeriodic($base + $t, 0, NULL, function ($w, $r)
 		use (&$id, &$prev, $base, $i, $t) {
 			$now = Ev::now();
 
 
 			$prev = $now;
 		});
-	$periodic->start();
+	$tmp_periodic->start();
+	$periodic[] = $tmp_periodic;
 }
 
 echo "ok 1\n";
 Ev::run();
 echo "ok 32\n";
+
+$timer = null;
+$periodic = null;
 ?>
 --EXPECTF--
 ok 1

File tests/06_keepalive.phpt

 Check for EvWatcher::keepalive() functionality
 --FILE--
 <?php 
-error_reporting(0);
+//error_reporting(0);
 
 $timer = EvTimer::createStopped(1, 0.3, function ($w, $r) {
 	echo "ok 7\n";

File tests/09_loop_timer.phpt

 $base = $l->now();
 $prev = $l->now();
 
+$timer = array();
+$periodic = array();
+
 for ($i = 1; $i <= /*125*/25; ++$i) {
 	$t = $i * $i * 1.735435336;
 	$t -= (int) $t;
-	$timer = $l->timer($t, 0, function ($w, $r)
+	$timer[] = $l->timer($t, 0, function ($w, $r)
 		use (&$id, &$prev, $base, $i, $t, $fudge) {
 			$now = $w->getLoop()->now();
 
 
 	$t = $i * $i * 1.375475771;
 	$t -= (int) $t;
-	$periodic = $l->periodic($base + $t, 0, NULL, function ($w, $r)
+	$periodic[] = $l->periodic($base + $t, 0, NULL, function ($w, $r)
 		use (&$id, &$prev, $base, $i, $t, $fudge) {
 			$now = $w->getLoop()->now();
 
 $l->run();
 print "ok 152\n";
 
+$timer = null;
+$periodic = null;
 ?>
 --EXPECTF--
 ok 1

File tests/11_watcher_data.phpt

 
 	// store data as ref to object
 	$t1 = $loop->timer(1, 0, function () {});
-	$t1->data = &$i1;
+	$t1->data = $i1;
 
 	// store data as object
 	$t2 = $loop->timer(1, 0, function () {});
-	$t2->data = $i2;
+	$t2->data = &$i2;
 
 	echo "0";
 	$t1->stop();

File tests/12_watcher_leak.phpt

+<?php
+
+error_reporting(E_ALL);
+ini_set('memory_limit', '5M');
+
+$i = 0;
+$startedAt = microtime(TRUE);
+$callback = function() use (&$i, $startedAt) {
+    if (!(++$i % 2500)) {
+        echo vsprintf("%d\t%d\t%d\t%.4f\t%d\n", $last = [
+            "job" => $i,
+            "mem" => memory_get_usage(),
+            "real" => memory_get_usage(true),
+            "runtime" => $runtime = (microtime(true) - $startedAt),
+            "jps" => ceil($i / $runtime)
+        ]);
+    }
+};
+
+$n = 0;
+while (++$n < 10000) {
+    //$timer = EvTimer::createStopped(-1, 0, $callback);
+    $timer = new EvTimer(-1, 0, $callback);
+    //Ev::run(Ev::RUN_NOWAIT | Ev::RUN_ONCE);
+
+}
+// What is the expected behaviour?
+// timers are created and destructed continuosly in the loop.
+// So very few of them will be actually invoked(if any).
+// Isn't it?
+Ev::run();
+echo "n: $n\n";
 void php_ev_watcher_callback(EV_P_ ev_watcher *watcher, int revents)
 {
 	zval            **args[2];
-	zval             *key1;
 	zval             *key2;
 	zval             *retval_ptr;
 	zval             *self       = php_ev_watcher_self(watcher);
 	TSRMLS_FETCH_FROM_CTX(php_ev_watcher_thread_ctx(watcher));
 
 	/* libev might have stopped watcher */
-	if (php_ev_watcher_flags(watcher) & PHP_EV_WATCHER_FLAG_UNREFED
-			&& !ev_is_active(watcher)) {
+	if (UNEXPECTED(php_ev_watcher_flags(watcher) & PHP_EV_WATCHER_FLAG_UNREFED
+			&& !ev_is_active(watcher))) {
 		PHP_EV_WATCHER_REF(watcher);
 	}
 
 		PHP_EV_EXIT_LOOP(EV_A);
 	} else if (ZEND_FCI_INITIALIZED(*pfci)) {
 		/* Setup callback args */
-		key1 = self;
-		args[0] = &key1;
-		zval_add_ref(&key1);
+		args[0] = &self;
+		Z_ADDREF_P(self);
 
 		MAKE_STD_ZVAL(key2);
 		args[1] = &key2;
 		            "An error occurred while invoking the callback");
 		}
 
-		zval_ptr_dtor(&key1);
+		zval_ptr_dtor(&self);
 		zval_ptr_dtor(&key2);
 	}
 }
 
 	ev_init((ev_watcher *) w, (ZEND_FCI_INITIALIZED(*pfci) ? php_ev_watcher_callback : 0));
 
-	Z_ADDREF_P(self);
 	if (data) {
 		Z_ADDREF_P(data);
 	}
 
+#if 0
+	Z_ADDREF_P(self);
+#endif
+
 	php_ev_watcher_self(w)  = self;
 	php_ev_watcher_data(w)  = data;
 	php_ev_watcher_loop(w)  = o_loop;
-	php_ev_watcher_flags(w) = PHP_EV_WATCHER_FLAG_KEEP_ALIVE;
+	php_ev_watcher_flags(w) = PHP_EV_WATCHER_FLAG_KEEP_ALIVE | PHP_EV_WATCHER_FLAG_SELF_UNREFED;
 
 	PHP_EV_COPY_FCALL_INFO(php_ev_watcher_fci(w), php_ev_watcher_fcc(w), pfci, pfcc);
 
 
 #define php_ev_watcher_loop_ptr(w)   (php_ev_watcher_loop(w)->loop)
 
-#define PHP_EV_WATCHER_FLAG_KEEP_ALIVE 1
-#define PHP_EV_WATCHER_FLAG_UNREFED    2
-
-#define PHP_EV_WATCHER_UNREF(w)                                                            \
-    if (!(php_ev_watcher_flags(w) &                                                        \
-                (PHP_EV_WATCHER_FLAG_KEEP_ALIVE | PHP_EV_WATCHER_FLAG_UNREFED))            \
-            && ev_is_active(w)) {                                                          \
-        ev_unref(php_ev_watcher_loop(w)->loop);                                            \
-        php_ev_watcher_flags(w) |= PHP_EV_WATCHER_FLAG_UNREFED;                            \
-    }
-
-#define PHP_EV_WATCHER_REF(w)                                                              \
-    if (php_ev_watcher_flags(w) & PHP_EV_WATCHER_FLAG_UNREFED) {                           \
-        php_ev_watcher_flags(w) &= ~PHP_EV_WATCHER_FLAG_UNREFED;                           \
-        ev_ref(php_ev_watcher_loop(w)->loop);                                              \
-    }
+#define PHP_EV_WATCHER_FLAG_KEEP_ALIVE   (1<<0)
+#define PHP_EV_WATCHER_FLAG_UNREFED      (1<<1)
+#define PHP_EV_WATCHER_FLAG_SELF_UNREFED (1<<2)
+
+#define PHP_EV_WATCHER_SELF_UNREF(w)                                             \
+	do {                                                                         \
+		if (php_ev_watcher_self(w)   \
+				&& !(php_ev_watcher_flags(w) & PHP_EV_WATCHER_FLAG_SELF_UNREFED)) { \
+			zval_ptr_dtor(&php_ev_watcher_self(w));                              \
+        	php_ev_watcher_flags(w) |= PHP_EV_WATCHER_FLAG_SELF_UNREFED;         \
+		}                                                                        \
+	} while (0)
+
+#define PHP_EV_WATCHER_SELF_REF(w)                                        \
+    	if (php_ev_watcher_flags(w) & PHP_EV_WATCHER_FLAG_SELF_UNREFED) { \
+        	Z_ADDREF_P(php_ev_watcher_self(w));                           \
+        	php_ev_watcher_flags(w) &= ~PHP_EV_WATCHER_FLAG_SELF_UNREFED; \
+        }                                                                 \
+
+#define PHP_EV_WATCHER_UNREF(w)                                                     \
+	do {                                                                            \
+    	if (!(php_ev_watcher_flags(w) &                                             \
+                	(PHP_EV_WATCHER_FLAG_KEEP_ALIVE | PHP_EV_WATCHER_FLAG_UNREFED)) \
+            	&& ev_is_active(w)) {                                               \
+        	ev_unref(php_ev_watcher_loop(w)->loop);                                 \
+        	php_ev_watcher_flags(w) |= PHP_EV_WATCHER_FLAG_UNREFED;                 \
+    	}                                                                           \
+		PHP_EV_WATCHER_SELF_REF(w);                                               \
+	} while (0)
+
+#define PHP_EV_WATCHER_REF(w)                                             \
+	do {                                                                  \
+    	if (php_ev_watcher_flags(w) & PHP_EV_WATCHER_FLAG_UNREFED) {      \
+        	php_ev_watcher_flags(w) &= ~PHP_EV_WATCHER_FLAG_UNREFED;      \
+        	ev_ref(php_ev_watcher_loop(w)->loop);                         \
+    	}                                                                 \
+		PHP_EV_WATCHER_SELF_UNREF(w);                                       \
+    } while (0)
+
 
 #define PHP_EV_WATCHER_STOP(t, w)                                                          \
     do {                                                                                   \