Memory leak with closures

[redbullmarky]

One more whilst you're on a roll :)
I revisited one of the scripts that's now working. Ran it through valgrind and getting 32,000bytes lost with 100 iterations. On more complex scripts it can get pretty hefty. If i return an anonymous class, for example, from my8::getthing(), it's perfectly fine.

<?php
class my8 extends \V8Js
{	
	public function getthing() {
		return function() {
		};
	}
}
$j = new my8();
for($i = 0; $i < 100; $i++) {
	$r = $j->executeString('
	(() => {
		var a = PHP.getthing()
	})();	
	');
 	echo "done {$i}\n";
	gc_collect_cycles();
}

the output there is:

==944240== HEAP SUMMARY:
==944240==     in use at exit: 84,159 bytes in 268 blocks
==944240==   total heap usage: 26,314 allocs, 26,046 frees, 16,984,708 bytes allocated
==944240==
==944240== 32,000 bytes in 100 blocks are definitely lost in loss record 44 of 44
==944240==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==944240==    by 0x384F1C: __zend_malloc (zend_alloc.c:2975)
==944240==    by 0x3D54A2: zend_closure_new (zend_closures.c:484)
==944240==    by 0x3B03F8: _object_and_properties_init (zend_API.c:1417)
==944240==    by 0x3B03F8: object_init_ex (zend_API.c:1431)
==944240==    by 0x3D5A3F: zend_create_closure (zend_closures.c:675)
==944240==    by 0x40098A: ZEND_DECLARE_LAMBDA_FUNCTION_SPEC_CONST_UNUSED_HANDLER (zend_vm_execute.h:9509)
==944240==    by 0x430062: execute_ex (zend_vm_execute.h:53953)
==944240==    by 0x75C395D: xdebug_execute_ex (base.c:376)
==944240==    by 0x39FAFE: zend_call_function (zend_execute_API.c:812)
==944240==    by 0x7D513BA: v8js_call_php_func(_zend_object*, _zend_function*, v8::FunctionCallbackInfo<v8::Value> const&) (v8js_object_export.cc:158)
==944240==    by 0x837273C: ??? (in /usr/local/lib/libv8.so)
==944240==    by 0x8371C4A: ??? (in /usr/local/lib/libv8.so)
==944240==
==944240== LEAK SUMMARY:
==944240==    definitely lost: 32,000 bytes in 100 blocks
==944240==    indirectly lost: 0 bytes in 0 blocks
==944240==      possibly lost: 0 bytes in 0 blocks
==944240==    still reachable: 52,159 bytes in 168 blocks
==944240==         suppressed: 0 bytes in 0 blocks

Somewhere the closure isn't being cleaned up properly, i'd imagine? Been poking around v8js_weak_closure_callback but not come up with anything as yet.

[stesie]

Hmm, interesting. I wonder whether PHP cleans those up if php-v8js isn't involved at all 🤔

Have you tried returning closures from functions without v8js and been able to reproduce leaks there?

Also I don't no much (hmm, anything) about PHP internals of closures, i.e. if this might be expected. Maybe worth noting: zend_closure_new uses emalloc. Therefore memory is freed once PHP execution instance exits. That is if you run v8js in e.g. apache (or fpm) and every http call creats just a few closures, those will be freed if the http call ends.

But agreed, if you create thousands of closures within the same http call you'll have a problem :)

[redbullmarky]

@stesie when you say without v8js, do you mean just vanilla PHP?

<?php
function getclosure() {
	return function() {		
	};
}
$a = getclosure();

the above doesn't report anything leaky. V8Js, per iteration, is leaking about 360bytes in my example. if i return an array of two closures, the result is the memory leak is exactly double.
for us, we only return anonymous classes rather than PHP closures so it's not an issue here, just one of those holes to plug i guess :) i'll carry on having a look myself to see if i can get more info.

[redbullmarky]

v8js_free_storage never gets called for my example when the var a = is given a closure constructed in PHP. Removing that line, or changing the return type of 'getthing' to anything else, does end up with v8js_free_storage being called, and valgrind doesn't report any problems.

presumably that's just because the main object has a refcount because of the closure?

[redbullmarky]

and a bit more digging....

i changed my getthing() method to return a closure:

return Closure::fromCallable(function() { });

but that had the same result. Poking around in the docs, however, it states "Create and return a new anonymous function from given callback using the current scope".

So (I think, though i don't know much about this area) the closure is keeping a reference to the my8 instance, so v8js_free_storage is never called for it, which doesn't free up the remaining stuff.

If I 'unbind' the closure:

$closure = Closure::fromCallable(function() { });
$closure = $closure->bindTo(null);
return $closure;

then v8js_free_storage is called, everything appears to get cleaned up nicely, and valgrind likes it again.

return static function() {
};

also works; so the problem really is just the freeing up of closures that are bound

[redbullmarky]

@stesie if v8js_free_storage is only called when the main V8Js instance is GC’d but there are closures bound to it out in the ether, then the code within that cleans up closures will never be called. Or something like that. Am I along the right lines? Any hints as to the best resolution?

[redbullmarky]

Extra: it’s not just closures. A regular object that holds a reference to V8Js will also leak.

[colinmollenhour]

Extra: it’s not just closures. A regular object that holds a reference to V8Js will also leak.

Do you mean running this JS leaks memory even if the result object in PHP is discarded?

return {"foo":"bar"};

[redbullmarky]

Extra: it’s not just closures. A regular object that holds a reference to V8Js will also leak.

Do you mean running this JS leaks memory even if the result object in PHP is discarded?

s
return {"foo":"bar"};

No, it was when you return something from PHP-land into JS. So by regular objects, I meant an instantiated PHP one, so doing it this way (when applied in the same way as my first example):

class my8 extends \V8Js
{
   public function getthing()
   {
      return new SomeClass();
   }
}

I initially thought it was a closure/anon function issue, but it’s the same with objects like this too.

[colinmollenhour]

Ahh, thanks for clarifying. I have no idea how the internals work but I wonder if it could be fixed by returning a weakref and then getting the weakref value inside of the JS context?

class my8 extends \V8Js
{
   public function getthing()
   {
      return WeakReference::create(new SomeClass());
   }
}

[redbullmarky]

Given we have control over all of the PHP stuff, it’s worth a go as a workaround. Will try it out when I get a chance and feed back!

Ultimately though, it’s probably still something that would be ideally fixed within the library itself, but it’s good to have options and fallbacks :)
Cheers!