Edit report at https://bugs.php.net/bug.php?id=81027&edit=1
ID: 81027
Updated by: nikic@php.net
Reported by: gy dot dlcs at gmail dot com
Summary: WeakReference will cause memory leaks
Status: Analyzed
Type: Bug
Package: Unknown/Other Function
Operating System: Linux
PHP Version: 8.0.6
Block user comment: N
Private report: N
New Comment:
> We should be able to address this in the same way as the unserialize() return value, which can
> also return a dead cycle: We need to explicitly root the object in WeakReference::get().
While that might address another case, it doesn't help here: We perform GC when releasing
$this, which will drop the root anyway. We'd have to re-add the root after GC like we do for
live TMPVARs, but I don't think we can easily do this for the result variable of the active
opcode, because we do not know whether or not it has been initialized yet.
Previous Comments:
------------------------------------------------------------------------
[2021-08-12 10:30:33] nikic@php.net
Simpler reproducer:
// We should have enough iterations to trigger a GC run.
for ($i = 0; $i < 10000; $i++) {
$a = new stdClass();
$a->a = $a;
$wr = WeakReference::create($a);
unset($a);
// The WeakReference::get() result should be used, but immediately destroyed.
!$wr->get();
}
This will detect the leak:
/home/nikic/php/php-src/Zend/zend_objects.c(186) : Freeing 0x00007ffa5bc09c30 (40 bytes),
script=/home/nikic/php/php-src/t415.php
The problem is as follows: Cycle GC is triggered when destroying the $this of the
WeakReference::get() call. At that point the object is stored inside the call return value, so it
cannot be GCed. Then the return value gets destroyed, but as it is TMPVAL, the assumption is that it
does not need to be rooted. As such, the cycle leaks.
We should be able to address this in the same way as the unserialize() return value, which can also
return a dead cycle: We need to explicitly root the object in WeakReference::get().
------------------------------------------------------------------------
[2021-08-12 10:14:51] nikic@php.net
Okay, I clearly didn't wait long enough last time. There is indeed a leak here, but a very slow
one.
I used this modified script:
<?php
ini_set('memory_limit', '8M');
$m = memory_get_usage();
for ($i = 0; ; $i++) {
$m2 = memory_get_usage();
if ($m2 > $m) {
$m = $m2;
echo "$i: ", $m, "\n";
}
$a = new stdClass();
$a->a = $a;
$wr = WeakReference::create($a);
unset($a);
// Uncomment next line, memory will not leak
//$wr->get();
// Uncomment next line, memory will leak
!$wr->get();
}
This shows that on every GC cycle (every 10000 iterations) memory usage increases by 560 bytes:
...
5449999: 6453552
5459999: 6454112
5469999: 6454672
5479999: 6455232
...
------------------------------------------------------------------------
[2021-05-14 12:08:18] dharman@php.net
I can't reproduce it with 8.0.3 but I can reproduce it with the current snapshot from GIT
------------------------------------------------------------------------
[2021-05-13 11:11:13] gy dot dlcs at gmail dot com
The above test script takes about 1 minute to reproduce the problem, Have you waited long enough?
------------------------------------------------------------------------
[2021-05-11 09:26:31] cmb@php.net
Thanks for the valgrind report, but unfortunately this is useless.
The warnings at the top are bogus, and everything after the SIGINT
is irrelevant. Sorry for requesting it!
I wouldn't know how to proceed with this issue.
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
https://bugs.php.net/bug.php?id=81027
--
Edit this bug report at https://bugs.php.net/bug.php?id=81027&edit=1