Strange SegFault and some more thoughts

[vituko]

Hi again, Ive done some more tests and thought a little more about it.

Calling an undefined method -> SegFault instead of Fatal Error Found a new strange problem :

<?
$obj = new StdClass() ;
$ref = new WeakRef ($obj) ;
$obj = $ref -> get() ; //Seems to be the cause, some kind of "cyclic reference"?
//unset ($obj) ; //Avoids SegFault
unset ($ref) ; // Throws SegFault
?>

As a suggestion, to avoid dependence on an unstable extension, I thought on an alternative that just tracks "refcounts". The method get increments an internal count attribute, and its counterpart method free decreases it. If it is possible to extend WeakRef and just add an empty method free, the switch would be easy and the performance penalty often negligible. Instead of calling the free method, one could call a function free_WeakRef ($ref) ; and there, call the method, so this function could be silenced when working with real WeakRefs, but... I attach the example code :

<?
function linha ($cadea) {
        echo $cadea."<br/>\n" ;
        @ob_end_flush() ;
        flush() ;
}
if (! class_exists ('WeakRef')) {
        class WeakRef {
                private $obj ;
                public static $fake = 1 ;
                public function __construct ($obj) {
                        $this -> acq = 0 ;
                        if (($type = gettype ($obj)) != 'object') {
                                trigger_error (__CLASS__.'::__construct() expects parameter 1 to be object, '.$type.' given', E_USER_WARNING) ;
                                $this -> obj = null ;
                                $this -> refs = 0 ;
                        }
                        $this -> obj = $obj ;
                        $this -> refs = 1 ;
                }
                public function acquire() {
                        if (! $this -> refs || $this -> acq)
                                return false ;
                        $this -> acq = 1 ;
                        return true ;
                }
                public function release() {
                        if (! $this -> acq)
                                return false ;
                        if (! $this -> refs)
                                $this -> obj = null ;
                        $this -> acq = 0 ;
                        return true ;
                }
                public function free() {
                        if (! $this -> refs)
                                return false ;
                        $this -> refs -- ;
                        if (! $this -> refs && ! $this -> acq)
                                $this -> obj = null ;
                        return true ;
                }
                public function valid() {
                        if (! $this -> refs && ! $this -> acq)
                                return false ;
                        return true ;
                }
                public function get() {
                        if ($this -> refs || $this -> acq)
                                $this -> refs ++ ;
                        return $this -> obj ;
                }
                public function __destruct() {
                        linha ('DESTR_weak') ;
                }
        }
        function free_WeakRef ($obj) {
                $obj -> free() ;
        }
} else {
        function free_WeakRef ($obj) {}
}
class class1 {
        private static $insts = array() ;
        public function __construct ($context) {
                $this -> context = $context ;
        }
        public static function inst ($context) {
                if (! isset (self::$insts [$context])/* || ! self::$insts [$this -> context] -> valid()*/)
                        self::$insts [$context] = new WeakRef ($inst = new self ($context)) ;
                else
                        $inst = self::$insts [$context] -> get() ;
                return $inst ;
        }
        public function __destruct() {
                unset (self::$insts [$this -> context]) ;
                linha ('DESTR_obj') ;
        }
        public function free() {
                //if (isset (WeakRef::fake)) { // Agnostic to implementation
                free_WeakRef (self::$insts [$this -> context]) ;
                /*
                //Implemented throw free_WeakRef and __destruct, old way :
                if (! isset (self::$insts [$this -> context]) || ! self::$insts [$this -> context] -> valid())
                        return false ;
                self::$insts [$this -> context] -> free() ;
                if (! self::$insts [$this -> context] -> valid())
                        unset (self::$insts [$this -> context]) ;
                */
        }
}
$obj1 = class1::inst ('context1') ;
$obj1 -> free() ;
unset ($obj1) ; // ->> Object is freed
linha ('FIN') ;
?>

Another interesting case for allowing extensions, a WeakRef wrapper specific for a class class1 : WR_class1

<?
$wr_obj1 = new WR_class1 ($id) ;
$aux = $wr_obj1 -> get() ;
?>

The wrapper class could store the id and guarantee that the get will always return a valid object, storing the $id and reinstantiating when the object gets lost. Further with this idea, delegate what to do when a ref becomes invalid, the wrapper could hide completely this extra step throw overloading methods, so $wrapper -> get() -> method() could become $wrapper -> method().

Another reason for not being a final class, after session serialization the WeakRef becomes always invalid, even when the object gets also independently serialized. In the same case, strong references are kept after this process. Just by extending WeakReaf, this could be avoided with custom magic sleep/wakeup methods. But could be also be considered a bug, or a desired behavior.

I dont know a lot about extensions, and nothing when OOP comes into play, but I think that making this function extendable could be a plus and just a last thought, exposing an update method, maybe protected to recover a weakly referenced object when lost, would be needed.

[colder]

Hi,

I will take a look at the segfault, thanks for reporting this.

Your userland implementation is a bit dubious, you simply can't have what weakref provides in userland because there is no way of tricking the GC that way.

If your implementation requires manual calls to track references, then you simply lose all the benefits of weakref.

I don't believe you need to subclass WeakRef to provide a wrapper on it, like you propose, you can simply define a class that contains a weakref property.

Best,

[Jille]

I was just implementing a testcase for probably the same problem. It is a lot more complicated but is usable with the testframework and can be simply stripped down.

https://github.com/Jille/php-weakref/blob/master/tests/weakref_007.phpt

[Jille]

In some earlier version I deducted the problem to be that the module's RSHUTDOWN was called before the destructors of all objects. I don't know whether that still applies.

[Jille]

I do have a backtrace of the current version.

(gdb) bt
#0  0x083424d6 in wr_store_detach (intern=0xb6df8094, ref_handle=7) at /usr/src/php-5.4.0-r110/ext/weakref/php_weakref.c:123
#1  0x08342abd in wr_weakref_object_free_storage (object=0xb6df8094) at /usr/src/php-5.4.0-r110/ext/weakref/wr_weakref.c:78
#2  0x083f871d in zend_objects_store_del_ref_by_handle_ex (handle=8, handlers=0x89bbc40) at /usr/src/php-5.4.0-r110/Zend/zend_objects_API.c:220
#3  0x083f8779 in zend_objects_store_del_ref (zobject=0xb6d8f524) at /usr/src/php-5.4.0-r110/Zend/zend_objects_API.c:172
#4  0x083c8e89 in _zval_ptr_dtor (zval_ptr=0xb6da1f24) at /usr/src/php-5.4.0-r110/Zend/zend_variables.h:35
#5  0x083e1f25 in zend_hash_destroy (ht=0xb6d8f9f4) at /usr/src/php-5.4.0-r110/Zend/zend_hash.c:560
#6  0x083d5bc6 in _zval_dtor_func (zvalue=0xb6d8f578) at /usr/src/php-5.4.0-r110/Zend/zend_variables.c:43
#7  0x083c8e89 in _zval_ptr_dtor (zval_ptr=0xb70395bc) at /usr/src/php-5.4.0-r110/Zend/zend_variables.h:35
#8  0x083f3961 in zend_object_std_dtor (object=0xb6e1c78c) at /usr/src/php-5.4.0-r110/Zend/zend_objects.c:54
#9  0x083f39a0 in zend_objects_free_object_storage (object=0xb6e1c78c) at /usr/src/php-5.4.0-r110/Zend/zend_objects.c:137
#10 0x083f871d in zend_objects_store_del_ref_by_handle_ex (handle=3, handlers=0x8936a80) at /usr/src/php-5.4.0-r110/Zend/zend_objects_API.c:220
#11 0x083f8779 in zend_objects_store_del_ref (zobject=0xb703bf2c) at /usr/src/php-5.4.0-r110/Zend/zend_objects_API.c:172
#12 0x083c8e89 in _zval_ptr_dtor (zval_ptr=0xb6e22368) at /usr/src/php-5.4.0-r110/Zend/zend_variables.h:35
#13 0x083e1f25 in zend_hash_destroy (ht=0xb6e22224) at /usr/src/php-5.4.0-r110/Zend/zend_hash.c:560
#14 0x083d5bc6 in _zval_dtor_func (zvalue=0xb703bf90) at /usr/src/php-5.4.0-r110/Zend/zend_variables.c:43
#15 0x083c8e89 in _zval_ptr_dtor (zval_ptr=0xb6e1ed00) at /usr/src/php-5.4.0-r110/Zend/zend_variables.h:35
#16 0x083f3961 in zend_object_std_dtor (object=0xb6e1fe90) at /usr/src/php-5.4.0-r110/Zend/zend_objects.c:54
#17 0x083f39a0 in zend_objects_free_object_storage (object=0xb6e1fe90) at /usr/src/php-5.4.0-r110/Zend/zend_objects.c:137
#18 0x083f8aaa in zend_objects_store_free_object_storage (objects=0x89be434) at /usr/src/php-5.4.0-r110/Zend/zend_objects_API.c:92
#19 0x083cb6e9 in shutdown_executor () at /usr/src/php-5.4.0-r110/Zend/zend_execute_API.c:297
#20 0x083d6b34 in zend_deactivate () at /usr/src/php-5.4.0-r110/Zend/zend.c:934
#21 0x0837e0db in php_request_shutdown (dummy=0x0) at /usr/src/php-5.4.0-r110/main/main.c:1786
#22 0x084756ab in do_cli (argc=2, argv=0xbfa0c984) at /usr/src/php-5.4.0-r110/sapi/cli/php_cli.c:1171
#23 0x084769bd in main (argc=2, argv=0xbfa0c984) at /usr/src/php-5.4.0-r110/sapi/cli/php_cli.c:1358
(gdb) frame 1
(gdb) print weakref_globals.store
$1 = (wr_store *) 0x0
(gdb) print *((wr_weakref_object *)object)->ref 
$2 = {value = {lval = 7, dval = 2.3521443565367845e-267, str = {val = 0x7 <Address 0x7 out of bounds>, len = 143878784}, ht = 0x7, obj = {handle = 7, handlers = 0x8936a80}}, refcount__gc = 3, 
  type = 5 '\005', is_ref__gc = 0 '\0'}

[colder]

The problem is related to the double-indirection zval->object.

An object can be referenced by multiple zvals with various refcounts, the problem occurs when the weakref tracks a zval that gets collected, while another zval is still pointing to the object. When the object disapears, the zval used by weakref has been freed and thus points to invalid memory.

The weakref store needs to track multiple zvals as references and instrument add_ref/del_ref handlers to keep track of them. It will require some important modifications of the code.

[vituko]

Well, if you cant trust in an unstable extension, or just want to support scenarios with limited extension availability you have three choices : 1- Forget ORM and such extensively object using schemes or always force short running time cycles scripts and forget caching too. 2- Infinity memory available 3- Free resources manually and its my approach. And using the same interface, makes the code work in both scenarios, with and without true weak references. Of course it can be added another level of wrapping, but it means more memory and more steps to access the true resources, think on a orm over a world wide geographical db. And whats about the serialization problem? Isnt it a bug? The second wrapper could solve this issue too, but with the same disadvantages. Whats wrong with allowing inheritance?

[colder]

Hi,

The problem mentioned above should be fixed now in master. The relevant tests:

https://github.com/colder/php-weakref/blob/master/tests/weakref_008.phpt https://github.com/colder/php-weakref/blob/master/tests/weakref_009.phpt

are now passing.

Vituko: my point is that if you do things manually, you don't need weakref at all. It's just that simple. Weakref is for people that don't want to do things manually. And for the unstable argument, it's obviously going to be stable at some point.

As for inheritance, I typically disable it at first because if I don't, I have to handle all the possible cases where people did not extend the class correctly (i.e. not calling the parent method) which in turn means that the class will misbehave, leading to additional crashes. Many of such cases did happen with SPL.

For Serialization, it would definitely be possible to handle it. I wouldn't say it's a bug, just a missing feature.

[vituko]

Thanks for your time and comments.

As I said, Im not aware of how much work it does mean. Its sure that it will become stable, but when will it be included in most hosting providers? That was my point, availability.

But well, points of view, of course, your work, your timetable, your aims.

[colder]

As a pecl extension, it is unlikely to become widespread anytime soon. For it to become widespread, it should be shipped with php. But this is another discussion :)