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
  [email protected]
  [email protected]