chobie / php-uv

libuv php extension
184 stars 21 forks source link

[WIP] - Libuv 1.0 Support #59

Open steverhoades opened 9 years ago

steverhoades commented 9 years ago

Updates are based on the migration guide: https://github.com/joyent/libuv/blob/a4f88760be1838603fe2eae89a651066cc42eedd/docs/src/migration_010_100.rst

There are still some segfault/segabrt issues that i am trying to get re-produceable tests in place for, leaving this here so that those with more experience can provide feedback.

steverhoades commented 9 years ago

This issue is rather strange, so I am documenting it here.

The following code works fine with no segfaults.

 <?php
 $loop = uv_loop_new();

 $timerHandle = uv_timer_init($loop);
 $innerCallback = function($timerHandle) {
     echo "OK";
     uv_timer_stop($timerHandle);
     uv_unref($timerHandle);
 };
 $outerCallback = function() use ($timerHandle, $innerCallback) {
     call_user_func($innerCallback, $timerHandle);
 };
 uv_timer_start($timerHandle, 1000, 1000, $outerCallback);

 uv_run($loop, UV::RUN_ONCE);

However this code, which performs the same functionality except inside a class causes a segfault on php_shutdown.

 <?php
 class test 
 {
     private $timers;
     private $loop;

     public function __construct()
     {
         $this->loop = uv_loop_new();
         $this->timers = new SplObjectStorage();
     }

     public function addTimer($timer)
     {
         $resource = uv_timer_init($this->loop);

         $this->timers->attach($timer, $resource);
         $outerCallback = function() use ($timer) {
             call_user_func($timer->callback, $this->timers[$timer]);
         };

         uv_timer_start($resource, 1000, 1000, $outerCallback);
     }

     public function run()
     {
         uv_run($this->loop, UV::RUN_ONCE);
     }   
 }

 class timer 
 {
     public $callback;
 }

 $innerCallback = function($timerHandle) {
     echo "OK";
     uv_timer_stop($timerHandle);
     uv_unref($timerHandle);
 };
 $timer = new timer();
 $timer->callback = $innerCallback;

 $test = new test();
 $test->addTimer($timer);
 $test->run();

Removing the uv_unref call fixes the issue with the above. Code should be updated so that it does not segfault.

Stack trace:

#0  uv_walk (loop=0x7ffff5dfa268, walk_cb=0x7ffff6181ffd <close_walk_cb>, arg=0x0) at src/uv-common.c:324
#1  0x00007ffff6182070 in destruct_uv_loop (rsrc=0x7ffff5dfbef0, tsrm_ls=0xff6ee0) at /home/vagrant/cpackages/php-uv/php_uv.c:1192
#2  0x0000000000878c55 in list_entry_destructor (ptr=0x7ffff5dfbef0) at /home/vagrant/cpackages/php-src/Zend/zend_list.c:183
#3  0x00000000008740e7 in i_zend_hash_bucket_delete (ht=0xffa840, p=0x7ffff5df9798) at /home/vagrant/cpackages/php-src/Zend/zend_hash.c:182
#4  0x00000000008759c0 in zend_hash_del_key_or_index (ht=0xffa840, arKey=0x0, nKeyLength=0, h=39, flag=1)
    at /home/vagrant/cpackages/php-src/Zend/zend_hash.c:526
#5  0x00000000008786d7 in _zend_list_delete (id=39, tsrm_ls=0xff6ee0) at /home/vagrant/cpackages/php-src/Zend/zend_list.c:57
#6  0x000000000085cd5c in _zval_dtor_func (zvalue=0x7ffff5dfa1e8, 
    __zend_filename=0xccc2d8 "/home/vagrant/cpackages/php-src/Zend/zend_execute.h", __zend_lineno=79)
    at /home/vagrant/cpackages/php-src/Zend/zend_variables.c:65
#7  0x0000000000845f08 in _zval_dtor (zvalue=0x7ffff5dfa1e8, __zend_filename=0xccc2d8 "/home/vagrant/cpackages/php-src/Zend/zend_execute.h", 
    __zend_lineno=79) at /home/vagrant/cpackages/php-src/Zend/zend_variables.h:35
#8  0x0000000000845ff7 in i_zval_ptr_dtor (zval_ptr=0x7ffff5dfa1e8, 
    __zend_filename=0xcd24c8 "/home/vagrant/cpackages/php-src/Zend/zend_objects.c", __zend_lineno=54, tsrm_ls=0xff6ee0)
    at /home/vagrant/cpackages/php-src/Zend/zend_execute.h:79
#9  0x0000000000848197 in _zval_ptr_dtor (zval_ptr=0x7ffff5df9b00, 
    __zend_filename=0xcd24c8 "/home/vagrant/cpackages/php-src/Zend/zend_objects.c", __zend_lineno=54)
    at /home/vagrant/cpackages/php-src/Zend/zend_execute_API.c:427
#10 0x000000000089c9aa in zend_object_std_dtor (object=0x7ffff5dfa068, tsrm_ls=0xff6ee0)
    at /home/vagrant/cpackages/php-src/Zend/zend_objects.c:54
#11 0x000000000089cfba in zend_objects_free_object_storage (object=0x7ffff5dfa068, tsrm_ls=0xff6ee0)
    at /home/vagrant/cpackages/php-src/Zend/zend_objects.c:137
#12 0x00000000008a5160 in zend_objects_store_del_ref_by_handle_ex (handle=12, handlers=0xfddc40, tsrm_ls=0xff6ee0)
    at /home/vagrant/cpackages/php-src/Zend/zend_objects_API.c:226
#13 0x00000000008a4ce0 in zend_objects_store_del_ref (zobject=0x7ffff5dfaa10, tsrm_ls=0xff6ee0)
    at /home/vagrant/cpackages/php-src/Zend/zend_objects_API.c:178
#14 0x000000000085cd32 in _zval_dtor_func (zvalue=0x7ffff5dfaa10, 
    __zend_filename=0xccc2d8 "/home/vagrant/cpackages/php-src/Zend/zend_execute.h", __zend_lineno=79)
    at /home/vagrant/cpackages/php-src/Zend/zend_variables.c:57
#15 0x0000000000845f08 in _zval_dtor (zvalue=0x7ffff5dfaa10, __zend_filename=0xccc2d8 "/home/vagrant/cpackages/php-src/Zend/zend_execute.h", 
    __zend_lineno=79) at /home/vagrant/cpackages/php-src/Zend/zend_variables.h:35
#16 0x0000000000845ff7 in i_zval_ptr_dtor (zval_ptr=0x7ffff5dfaa10, 
    __zend_filename=0xcce480 "/home/vagrant/cpackages/php-src/Zend/zend_variables.c", __zend_lineno=187, tsrm_ls=0xff6ee0)
    at /home/vagrant/cpackages/php-src/Zend/zend_execute.h:79
#17 0x0000000000848197 in _zval_ptr_dtor (zval_ptr=0x7ffff5dff970, 
    __zend_filename=0xcce480 "/home/vagrant/cpackages/php-src/Zend/zend_variables.c", __zend_lineno=187)
    at /home/vagrant/cpackages/php-src/Zend/zend_execute_API.c:427
#18 0x000000000085d165 in _zval_ptr_dtor_wrapper (zval_ptr=0x7ffff5dff970) at /home/vagrant/cpackages/php-src/Zend/zend_variables.c:187
#19 0x00000000008740e7 in i_zend_hash_bucket_delete (ht=0xffa738, p=0x7ffff5dff958) at /home/vagrant/cpackages/php-src/Zend/zend_hash.c:182
#20 0x00000000008741be in zend_hash_bucket_delete (ht=0xffa738, p=0x7ffff5dff958) at /home/vagrant/cpackages/php-src/Zend/zend_hash.c:192
#21 0x00000000008762be in zend_hash_reverse_apply (ht=0xffa738, apply_func=0x846ec3 <zval_call_destructor>, tsrm_ls=0xff6ee0)
    at /home/vagrant/cpackages/php-src/Zend/zend_hash.c:733
#22 0x0000000000847003 in shutdown_destructors (tsrm_ls=0xff6ee0) at /home/vagrant/cpackages/php-src/Zend/zend_execute_API.c:217
#23 0x0000000000860227 in zend_call_destructors (tsrm_ls=0xff6ee0) at /home/vagrant/cpackages/php-src/Zend/zend.c:933
#24 0x00000000007a2f7f in php_request_shutdown (dummy=0x0) at /home/vagrant/cpackages/php-src/main/main.c:1826
#25 0x000000000092913d in do_cli (argc=7, argv=0xff6cc0, tsrm_ls=0xff6ee0) at /home/vagrant/cpackages/php-src/sapi/cli/php_cli.c:1177
#26 0x0000000000929920 in main (argc=7, argv=0xff6cc0) at /home/vagrant/cpackages/php-src/sapi/cli/php_cli.c:1378
rdlowrey commented 9 years ago

This segfault isn't surprising. You're manually decrementing the reference count for the timer resource from one to zero inside your callback and this leads to the actual UV resource being cleared:

zend_list_delete(uv->resource_id);

This is fine in your procedural code because there are no other references to the timer resource that need to be garbage collected. But your class example is storing a reference to the timer resource inside the SplObjectStorage thing. So you've removed the resource entry as far as PHP is concerned but retained a reference to that deleted entry in your userland code. Now when PHP tries to garbage collect the referenced memory down the road you get a segfault because it's already gone!

In short, you're only asking for trouble if you start fiddling with refcounts in userland. Just call uv_timer_stop() and remove any references in your code to the timer resource created from uv_timer_init($loop);.

Honestly, unless someone has a compelling reason to expose uv_unref() to userland I'm not sure it makes sense to have this functionality available in the extension at all. The only thing I can see it doing is causing segfaults without any actual benefit. Just because libuv exposes a uv_unref() doesn't necessarily mean it belongs in userland PHP ...

Anybody else have thoughts on this?

steverhoades commented 9 years ago

@rdlowrey I owe you a beer my friend. I have been trying to track this down for several days. Removing the zend_list_delete calls in uv_unref fixes the segfaulting issues as these are cleared in destruct_uv and destruct_uv_loop - this allows for proper cleanup with libuv 1.0. I'll check this in shortly, thanks sooooo much!

rdlowrey commented 9 years ago

@steverhoades no problem man; I appreciate your efforts to bring the extension up to 1.0 compatibility!

:+1:

steverhoades commented 9 years ago

libuv 1.0 has been officially released: https://github.com/joyent/libuv/commit/feb2a9e6947d892f449b2770c4090f7d8c88381b

chobie commented 9 years ago

@rdlowrey @steverhoades Thank you! this patch is awesome! I'm playing this branch right now.

php tests/300-fs.php | hexdump -C
00000000  65 6c 6c 6f 0a 0a 65 6c  6c 6f 0a                 |ello..ello.|
0000000b

Hmm, This is my fault. static char uv_fs_read_buf[8192]; seems broken. probably this should be prepare a buffer for each uv_fs_read call.

I'd like to merge this and fix this problem later. I suppose libuv 1.0 brings big worth for us.

rdlowrey commented 9 years ago

@chobie Yeah I realized later that the buffer was stored there. I've since fixed this locally and have been using it to great effect. I'll try to update my PR with the working changes this week.

steverhoades commented 9 years ago

@rdlowrey fantastic! I haven't had any time recently to continue to look into this further however it's still something I would like to get done. Let me know if there is anything I can do to help.