search

laruence / php-lua

This extension embeds the lua interpreter and offers an OO-API to lua variables and functions.
http://pecl.php.net/package/lua
Other
149 stars 50 forks source link

Explicitly release held references to PHP callbacks #31

Closed sgolemon closed 6 years ago

sgolemon commented 6 years ago

Noticed the leak while running make test in a debug build. A few thoughts:

  1. Letting the callbacks leak is probably not the worst thing in the world since we're only cleaning them up at RSHUTDOWN anyway. So leaving out this patch would be marginally less correct even if marginally more performant. Marginally either way.

  2. Shouldn't these callable references exist on the Lua PHP object instance? That way when the interpreter falls out of scope and we shutdown the Lua runtime, we can opportunistically free these references earlier (and avoid the RSHUTDOWN callback).

laruence commented 6 years ago

for now, I am simply merge your fix to fix the leak. I will think about 2), which I prefer more..

thanks

laruence commented 6 years ago

hmm, there might be something wrong, static properties should be freed properly by Zend vm..

I will look deeper later.

laruence commented 6 years ago

I confirmed, static properties already be take cared in zend_cleanup_internal_class_data...

sgolemon commented 6 years ago

Ah, I think I know what's happening.

add_next_index_zval() ends up adding a ref automatically during the COPY_ZVAL call in zend_hash_add_or_update_i so the extra addref you have here https://github.com/laruence/php-lua/blob/c4c61f05bf297831ef6ccc61c7e53111614f3c8f/lua.c#L424 is probably what's causing the leak.

laruence commented 6 years ago

zend_hash_add_or_update_i calls ZVAL_COPY_VALUE, it won't increase the refcount

I think the problem should be in somewhere else :<