Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested CallbackFilterIterator is leaking memory #7958

Closed
mvorisek opened this issue Jan 17, 2022 · 3 comments
Closed

Nested CallbackFilterIterator is leaking memory #7958

mvorisek opened this issue Jan 17, 2022 · 3 comments

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Jan 17, 2022

Description

The following code:

https://3v4l.org/3nIOH

https://3v4l.org/YQIVJ/rfc#vgit.master

is expected to output:

 -- c 1
 Array
 (
     [0] => a
     [1] => b
 )
 -- d 1
 -- c 1
 Array
 (
     [0] => a
     [1] => b
 )
 -- d 1
 -- c 1
 Array
 (
     [0] => a
 )
+ -- d 1
 ----------------
- -- d 1

PHP Version

8.0.14, 8.1.1, master (all tested)

Operating System

linux

@mvorisek
Copy link
Contributor Author

mvorisek commented Feb 1, 2022

Can anyone from PHP team confirm this issue? It is probably related to afab9eb , the issue with CallbackFilterIterator is however fixed, thanks @nikic

mvorisek added a commit to mvorisek/php-src that referenced this issue Feb 1, 2022
@mvorisek mvorisek mentioned this issue Feb 1, 2022
mvorisek added a commit to mvorisek/php-src that referenced this issue Feb 1, 2022
@iluuu1994
Copy link
Member

iluuu1994 commented Feb 3, 2022

Doesn't seem like this is caused by afab9eb. In fact, it fixes the second iteration in your example, which also leaked.

Here's a somewhat condensed example:

class Action
{
    public \Iterator $iterator;

    public function __construct(array $data)
    {
        $this->iterator = new ArrayIterator($data);
        echo '-- c ' . spl_object_id($this) . "\n";
    }

    public function __destruct()
    {
        echo '-- d ' . spl_object_id($this) . "\n";
    }

    public function filter()
    {
        $this->iterator = new \CallbackFilterIterator($this->iterator, fn() => true);
        $this->iterator->rewind();
    }
}

$action = new Action(['a', 'b']);
$action->filter();
$action->filter();
print_r(iterator_to_array($action->iterator));
$action = null;
gc_collect_cycles();
-- c 1
Array
(
    [0] => a
    [1] => b
)
===DONE===
-- d 1
[Thu Feb  3 15:51:02 2022]  Script:  './ext/spl/tests/bug-gh7958.php'
./Zend/zend_closures.c(505) :  Freeing 0x00007f5edee6b900 (328 bytes), script=./ext/spl/tests/bug-gh7958.php
[Thu Feb  3 15:51:02 2022]  Script:  './ext/spl/tests/bug-gh7958.php'
./Zend/zend_objects_API.h(92) :  Freeing 0x00007f5edee84540 (176 bytes), script=./ext/spl/tests/bug-gh7958.php
[Thu Feb  3 15:51:02 2022]  Script:  './ext/spl/tests/bug-gh7958.php'
./Zend/zend_interfaces.c(206) :  Freeing 0x00007f5edee87280 (112 bytes), script=./ext/spl/tests/bug-gh7958.php
=== Total 3 memory leaks detected ===

It's caused by the reference to $this from the CallbackFilterIterator closure. Making the closure static solves the issue. It only happens for nested CallbackFilterIterator iterators.

@mvorisek
Copy link
Contributor Author

mvorisek commented Feb 4, 2022

It's caused by the reference to $this from the CallbackFilterIterator closure. Making the closure static solves the issue. It only happens for nested CallbackFilterIterator iterators.

Yes, it seems ref count is not properly decremented during GC cycle when CallbackFilterIterator is nested - https://3v4l.org/mNt0C/rfc#vgit.master

Can you please mark this issue as verified and do you think it can be fixed in PHP v8.0 or at least v8.1?

@mvorisek mvorisek changed the title LimitIterator is not GCable Nested CallbackFilterIterator is leaking memory Feb 4, 2022
@cmb69 cmb69 self-assigned this Feb 8, 2022
cmb69 added a commit to cmb69/php-src that referenced this issue Feb 17, 2022
We implement `zend_object_iterator_funcs.get_gc` for user iterators to
avoid the memory leak.
@cmb69 cmb69 linked a pull request Feb 17, 2022 that will close this issue
@cmb69 cmb69 closed this as completed in fb70460 Feb 21, 2022
cmb69 added a commit that referenced this issue Feb 21, 2022
* PHP-8.1:
  Fix GH-7958: Nested CallbackFilterIterator is leaking memory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@iluuu1994 @mvorisek @cmb69 and others