Fix Heap iterator value ownership and rewinding

README.md

@@ -21,6 +21,8 @@ Documentation is available on [php.net](https://www.php.net/manual/en/book.ds.ph
 - **Ds\Key** -- interface for custom key equality (replaces Hashable)
 
 `getIterator()` on `Seq`, `Map`, `Set`, and `Heap` returns an `InternalIterator` supporting the standard `Iterator` methods.
+Heap iterators retain a snapshot of the values present when created; `rewind()` restarts that snapshot even if the original heap has changed.
+Calling `next()` or `rewind()` on the same iterator from its comparator throws a `RuntimeException`.
 
 You should also include the [polyfill](https://github.com/php-ds/polyfill) in your project for IDE integration.
 

src/php/iterators/php_heap_iterator.c

@@ -8,8 +8,11 @@ static void php_ds_heap_iterator_dtor(zend_object_iterator *iter)
 {
     php_ds_heap_iterator_t *iterator = (php_ds_heap_iterator_t *) iter;
 
-    ds_heap_free(iterator->heap);
+    if (iterator->heap) {
+        ds_heap_free(iterator->heap);
+    }
     DTOR_AND_UNDEF(&iterator->intern.data);
+    ds_heap_release(iterator->snapshot);
     OBJ_RELEASE(iterator->object);
 }
 
@@ -20,36 +23,61 @@ static int php_ds_heap_iterator_valid(zend_object_iterator *iter)
 
 static zval *php_ds_heap_iterator_get_current_data(zend_object_iterator *iter)
 {
-    return &iter->data;
+    return Z_ISUNDEF(iter->data) ? NULL : &iter->data;
 }
 
 static void php_ds_heap_iterator_get_current_key(zend_object_iterator *iter, zval *key)
 {
-    ZVAL_LONG(key, ((php_ds_heap_iterator_t *) iter)->position);
+    if (Z_ISUNDEF(iter->data)) {
+        ZVAL_NULL(key);
+    } else {
+        ZVAL_LONG(key, ((php_ds_heap_iterator_t *) iter)->position);
+    }
 }
 
 static void php_ds_heap_iterator_set_current(php_ds_heap_iterator_t *iterator)
 {
-    if (DS_HEAP_IS_EMPTY(iterator->heap)) {
-        ZVAL_UNDEF(&iterator->intern.data);
-    } else {
+    DTOR_AND_UNDEF(&iterator->intern.data);
+
+    if (!DS_HEAP_IS_EMPTY(iterator->heap)) {
         ds_heap_pop(iterator->heap, &iterator->intern.data, iterator->compare);
-        Z_TRY_DELREF_P(&iterator->intern.data);
     }
 }
 
 static void php_ds_heap_iterator_move_forward(zend_object_iterator *iter)
 {
     php_ds_heap_iterator_t *iterator = (php_ds_heap_iterator_t *) iter;
+
+    if (iterator->running) {
+        ds_throw_exception(spl_ce_RuntimeException,
+            "Cannot modify a heap iterator while it is executing");
+        return;
+    }
+
+    iterator->running = true;
     php_ds_heap_iterator_set_current(iterator);
     iterator->position++;
+    iterator->running = false;
 }
 
 static void php_ds_heap_iterator_rewind(zend_object_iterator *iter)
 {
     php_ds_heap_iterator_t *iterator = (php_ds_heap_iterator_t *) iter;
+
+    if (iterator->running) {
+        ds_throw_exception(spl_ce_RuntimeException,
+            "Cannot modify a heap iterator while it is executing");
+        return;
+    }
+
+    iterator->running = true;
+    if (iterator->heap) {
+        ds_heap_free(iterator->heap);
+    }
+    iterator->heap = ds_heap_clone(iterator->snapshot);
     php_ds_heap_iterator_set_current(iterator);
     iterator->position = 0;
+    iterator->running = false;
 }
 
 static zend_object_iterator_funcs iterator_funcs = {
@@ -79,7 +107,8 @@ zend_object_iterator *php_ds_heap_get_iterator(zend_class_entry *ce, zval *objec
     ZVAL_UNDEF(&iterator->intern.data);
 
     iterator->intern.funcs = &iterator_funcs;
-    iterator->heap         = ds_heap_clone(obj->heap);
+    iterator->snapshot     = obj->heap;
+    iterator->snapshot->refs++;
     iterator->compare      = php_ds_heap_get_compare(obj);
     iterator->object       = Z_OBJ_P(object);
     iterator->position     = 0;

src/php/iterators/php_heap_iterator.h

@@ -7,9 +7,11 @@
 typedef struct _php_ds_heap_iterator_t {
     zend_object_iterator       intern;
     zend_object               *object;
+    ds_heap_t                 *snapshot; // Shared heap retained for rewinding.
     ds_heap_t                 *heap;    // Cloned heap — popped during iteration.
     ds_heap_compare_func_t     compare;
     zend_long                  position;
+    bool                       running; // Prevent modification from comparator callbacks.
 } php_ds_heap_iterator_t;
 
 zend_object_iterator *php_ds_heap_get_iterator(zend_class_entry *ce, zval *obj, int by_ref);

tests/Heap/getIterator_current.phpt

@@ -0,0 +1,57 @@
+--TEST--
+Heap: iterator owns its current value after the source heap is cleared
+--FILE--
+<?php
+$heap = new Ds\Heap([(object) ['value' => 1]]);
+$iterator = $heap->getIterator();
+$heap->clear();
+unset($heap);
+
+// Releasing a current() result must not free the iterator's current value.
+echo json_encode($iterator->current()), "\n";
+echo json_encode($iterator->current()), "\n";
+
+// A returned value must also survive destruction of the iterator.
+$value = $iterator->current();
+$reference = WeakReference::create($value);
+unset($iterator);
+echo json_encode($value), "\n";
+unset($value);
+var_dump($reference->get());
+
+// Advancing and destroying the iterator must release all owned values.
+$values = [(object) ['value' => 1], (object) ['value' => 2]];
+$references = array_map(WeakReference::create(...), $values);
+$heap = new Ds\Heap($values);
+$iterator = $heap->getIterator();
+unset($values);
+$heap->clear();
+while ($iterator->valid()) {
+    echo json_encode($iterator->current()), "\n";
+    $iterator->next();
+}
+var_dump($iterator->current(), $iterator->key());
+unset($iterator);
+foreach ($references as $reference) {
+    var_dump($reference->get());
+}
+
+// An iterator can be destroyed before it has ever been rewound.
+$heap = new Ds\Heap([new stdClass()]);
+$reference = WeakReference::create($heap->peek());
+$iterator = $heap->getIterator();
+unset($heap, $iterator);
+var_dump($reference->get());
+?>
+--EXPECT--
+{"value":1}
+{"value":1}
+{"value":1}
+NULL
+{"value":2}
+{"value":1}
+NULL
+NULL
+NULL
+NULL
+NULL

tests/Heap/getIterator_reentrant.phpt

@@ -0,0 +1,99 @@
+--TEST--
+Heap: comparator callbacks cannot advance or rewind the same iterator
+--FILE--
+<?php
+function checkReentry(string $outer, string $inner, bool $catchInside): void
+{
+    echo "$outer -> $inner, caught ", $catchInside ? 'inside' : 'outside', "\n";
+    $iterator = null;
+    $attempt = false;
+    $heap = new Ds\Heap([1, 2, 3, 4, 5], static function ($a, $b) use (
+        &$iterator, &$attempt, $inner, $catchInside
+    ) {
+        if ($attempt) {
+            $attempt = false;
+            if ($catchInside) {
+                try {
+                    $iterator->$inner();
+                } catch (RuntimeException $e) {
+                    echo $e->getMessage(), "\n";
+                }
+            } else {
+                $iterator->$inner();
+            }
+        }
+        return $a <=> $b;
+    });
+    $iterator = $heap->getIterator();
+    if ($outer === 'next') {
+        $iterator->rewind();
+    }
+
+    $attempt = true;
+    try {
+        $iterator->$outer();
+    } catch (RuntimeException $e) {
+        echo $e->getMessage(), "\n";
+    }
+
+    if ($catchInside) {
+        // A rejected nested operation must not change the key or skip a value.
+        echo $iterator->key(), ':', $iterator->current(), "\n";
+        $iterator->next();
+        echo $iterator->key(), ':', $iterator->current(), "\n";
+    }
+
+    // The guard must be released even when the comparator propagates an exception.
+    echo json_encode(iterator_to_array($iterator)), "\n";
+    echo json_encode($heap->toArray()), "\n";
+    $iterator = null; // Break the reference held by the comparator.
+}
+
+foreach (['rewind', 'next'] as $outer) {
+    foreach (['rewind', 'next'] as $inner) {
+        checkReentry($outer, $inner, true);
+        checkReentry($outer, $inner, false);
+    }
+}
+?>
+--EXPECT--
+rewind -> rewind, caught inside
+Cannot modify a heap iterator while it is executing
+0:5
+1:4
+[5,4,3,2,1]
+[5,4,3,2,1]
+rewind -> rewind, caught outside
+Cannot modify a heap iterator while it is executing
+[5,4,3,2,1]
+[5,4,3,2,1]
+rewind -> next, caught inside
+Cannot modify a heap iterator while it is executing
+0:5
+1:4
+[5,4,3,2,1]
+[5,4,3,2,1]
+rewind -> next, caught outside
+Cannot modify a heap iterator while it is executing
+[5,4,3,2,1]
+[5,4,3,2,1]
+next -> rewind, caught inside
+Cannot modify a heap iterator while it is executing
+1:4
+2:3
+[5,4,3,2,1]
+[5,4,3,2,1]
+next -> rewind, caught outside
+Cannot modify a heap iterator while it is executing
+[5,4,3,2,1]
+[5,4,3,2,1]
+next -> next, caught inside
+Cannot modify a heap iterator while it is executing
+1:4
+2:3
+[5,4,3,2,1]
+[5,4,3,2,1]
+next -> next, caught outside
+Cannot modify a heap iterator while it is executing
+[5,4,3,2,1]
+[5,4,3,2,1]

tests/Heap/getIterator_rewind.phpt

@@ -0,0 +1,50 @@
+--TEST--
+Heap: iterator rewind restores the original snapshot
+--FILE--
+<?php
+$heap = new Ds\Heap([1, 3, 2]);
+$iterator = $heap->getIterator();
+
+// Mutation before the first rewind must not change the iterator's snapshot.
+$heap->clear();
+$heap->push(99);
+
+$iterator->rewind();
+echo $iterator->key(), ':', $iterator->current(), "\n";
+$iterator->rewind();
+echo $iterator->key(), ':', $iterator->current(), "\n";
+$iterator->next();
+echo $iterator->key(), ':', $iterator->current(), "\n";
+$iterator->rewind();
+echo $iterator->key(), ':', $iterator->current(), "\n";
+
+// foreach rewinds the same iterator, including after it has been exhausted.
+echo json_encode(iterator_to_array($iterator)), "\n";
+echo json_encode(iterator_to_array($iterator)), "\n";
+echo json_encode($heap->toArray()), "\n";
+
+// Rewinding must preserve the custom comparator's ordering too.
+$heap = new Ds\Heap([3, 1, 2], fn($a, $b) => $b <=> $a);
+$iterator = $heap->getIterator();
+echo json_encode(iterator_to_array($iterator)), "\n";
+echo json_encode(iterator_to_array($iterator)), "\n";
+
+$iterator = (new Ds\Heap())->getIterator();
+$iterator->rewind();
+$iterator->rewind();
+var_dump($iterator->valid());
+var_dump($iterator->current(), $iterator->key());
+?>
+--EXPECT--
+0:3
+0:3
+1:2
+0:3
+[3,2,1]
+[3,2,1]
+[99]
+[1,2,3]
+[1,2,3]
+bool(false)
+NULL
+NULL