Replace wrapTransversable generators to prevent memory leaks

[GromNaN]

Q	A
Type	bug
BC Break	no
Fixed issues	#2105

Problem

A non-static generator created with a method of a class instance holds the reference to this instance. When assigned to a property of this instance, this creates a circular reference (instance -> generator -> instance). The reference is destroyed only when the generator is exhausted.

Most of the time, when the memory limit is sufficient, this is not an issue. The garbage collector should remove this circular references, but it runs automatically only when reaching the GC_THRESHOLD_DEFAULT.

When working with a MongoDB Cursor, this is a single object which can fill up a lot of memory, that explains why gc_collect_cycles() is not run automatically.

Solution

Using IteratorIterator instead of Generator, we remove this circular reference . Also, I find the code simpler.
Same as mongodb/mongo-php-library#694

Demonstration

In the following example, if gc_collect_cycles() is not called in the loop, we can see the memory leak. When calling gc_collect_cycles() explicitly, we can see 300 "collected" references. The memory leak disappears when the Generator is replaced by an IteratorIterator.

Source code

<?php

namespace Hell;

class Iterator implements \Iterator
{
    private iterable $iterator;

    public function __construct(\Traversable $data)
    {
        $this->iterator = $this->wrapTraversable($data);
        //$this->iterator = new \IteratorIterator($data);
    }

    public function current(): mixed
    {
        return $this->iterator->current();
    }

    public function next(): void
    {
        $this->iterator->next();
    }

    public function key(): mixed
    {
        return $this->iterator->key();
    }

    public function valid(): bool
    {
        return $this->iterator->valid();
    }

    public function rewind(): void
    {
        $this->iterator->rewind();
    }

    private function wrapTraversable(iterable $data): \Generator
    {
        foreach ($data as $key => $value) {
            yield $key => $value;
        }
    }
}

foreach (range(0, 100) as $i) {
    $data = new \ArrayObject(array_fill(0, 1_000_00, bin2hex(random_bytes(1_000))));
    $iterator = new Iterator($data);
    $iterator->next();
    unset($data);
    echo (memory_get_usage()).PHP_EOL;
}

gc_collect_cycles();
print_r(gc_status());

[GromNaN]

Replaced by setting $iterator = null, as it's already done for UnrewindableIterator

[GromNaN]

This checks for a dead cursor.

[alcaeus]

Great work refactoring this. I've left a suggestion to refactor storeCurrentItems to consolidate some logic, but LGTM aside from that.