Re: [APC-DEV] refcount/is_ref commit

On Dec 28, 2006, at 2:16 AM, Gopal V wrote:

Hi, Looking at (http://news.php.net/php.pecl.cvs/6820)

-    /* deep-copying ensures that there is only one reference to this in memory */
-    (*dst)->refcount = 1;
-    (*dst)->is_ref = 0;
+    (*dst)->refcount = (*src)->refcount;
+    (*dst)->is_ref = (*src)->is_ref;

That could be a problem. When you copy *from* shm land, the zval->refcount used to be 1 because we never had circular references before. So this *might* leak (need more time to debug this), but there's another inconsistent part that might be more important.

Are we talking about memory leaks in PHP due to no cycle detection? (ie: any $a[] = &$a; type reference will leak in php), or something else? I don't believe we should have an APC shared memory leak, but that's something I should verify. Thanks for looking at this, definitely part of the commit that I was a bit unsure about.

Because you call my_copy_zval() before you reset this, the refcount++ inside that function is over-written with the src->refcount.

Thanks for pointing it out, I wish I had seen it. Also looking at it now it seems like I shouldn't be excluding the reset of the refcount. Wonder if the preferred solution is something more along the lines of (saying this without testing it out *too* much): --- a/apc_compile.c +++ b/apc_compile.c @@ -257,10 +257,9 @@ static zval** my_copy_zval_ptr(zval** ds

     if(dst_new != *dst) {
         deallocate(*dst);
         *dst = dst_new;
+    } else {
+        (*dst)->refcount = 1;
     }

-

-    (*dst)->refcount = (*src)->refcount;
-    (*dst)->is_ref = (*src)->is_ref;

     return dst;

}

Cheers, Gopal PS: every time *I* start working on apc, something like this ruins my

    day too :)

"If I learn from my mistakes, pretty soon I'll know everything." :-)

--You can't teach people to be lazy - either they have it, or they don't.

-Brian Shire shire@facebook.com shire@php.net