Skip to content

Commit cd25500

Browse files
arnaud-lbarnaud-lb
authored
Do not scan generator frames more than once (#15330)
1 parent 7878a2c commit cd25500

10 files changed

+357
-35
lines changed

‎Zend/tests/gh15330-001.phpt

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
--TEST--
2+
GH-15330 001: Do not scan generator frames more than once
3+
--FILE--
4+
<?php
5+
6+
class It implements \IteratorAggregate
7+
{
8+
public function getIterator(): \Generator
9+
{
10+
yield 'foo';
11+
Fiber::suspend();
12+
var_dump("not executed");
13+
}
14+
}
15+
16+
function f() {
17+
var_dump(yield from new It());
18+
}
19+
20+
$iterable = f();
21+
22+
$fiber = new Fiber(function () use ($iterable) {
23+
var_dump($iterable->current());
24+
$iterable->next();
25+
var_dump("not executed");
26+
});
27+
28+
$ref = $fiber;
29+
30+
$fiber->start();
31+
32+
gc_collect_cycles();
33+
34+
?>
35+
==DONE==
36+
--EXPECT--
37+
string(3) "foo"
38+
==DONE==

‎Zend/tests/gh15330-002.phpt

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
--TEST--
2+
GH-15330 002: Do not scan generator frames more than once
3+
--FILE--
4+
<?php
5+
6+
function g() {
7+
yield 'foo';
8+
Fiber::suspend();
9+
}
10+
11+
function f() {
12+
var_dump(yield from g());
13+
}
14+
15+
$iterable = f();
16+
17+
$fiber = new Fiber(function () use ($iterable) {
18+
var_dump($iterable->current());
19+
$iterable->next();
20+
var_dump("not executed");
21+
});
22+
23+
$ref = $fiber;
24+
25+
$fiber->start();
26+
27+
gc_collect_cycles();
28+
29+
?>
30+
==DONE==
31+
--EXPECT--
32+
string(3) "foo"
33+
==DONE==

‎Zend/tests/gh15330-003.phpt

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
--TEST--
2+
GH-15330 003: Do not scan generator frames more than once
3+
--FILE--
4+
<?php
5+
6+
class It implements \IteratorAggregate
7+
{
8+
public function getIterator(): \Generator
9+
{
10+
yield 'foo';
11+
Fiber::suspend();
12+
var_dump("not executed");
13+
}
14+
}
15+
16+
class Canary {
17+
public function __construct(public mixed $value) {}
18+
public function __destruct() {
19+
var_dump(__METHOD__);
20+
}
21+
}
22+
23+
function f($canary) {
24+
var_dump(yield from new It());
25+
}
26+
27+
$canary = new Canary(null);
28+
29+
$iterable = f($canary);
30+
31+
$fiber = new Fiber(function () use ($iterable, $canary) {
32+
var_dump($canary, $iterable->current());
33+
$iterable->next();
34+
var_dump("not executed");
35+
});
36+
37+
$canary->value = $fiber;
38+
39+
$fiber->start();
40+
41+
$iterable->current();
42+
43+
$fiber = $iterable = $canary = null;
44+
45+
gc_collect_cycles();
46+
47+
?>
48+
==DONE==
49+
--EXPECTF--
50+
object(Canary)#%d (1) {
51+
["value"]=>
52+
object(Fiber)#%d (0) {
53+
}
54+
}
55+
string(3) "foo"
56+
string(18) "Canary::__destruct"
57+
==DONE==

‎Zend/tests/gh15330-004.phpt

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
--TEST--
2+
GH-15330 004: Do not scan generator frames more than once
3+
--FILE--
4+
<?php
5+
6+
class Canary {
7+
public function __construct(public mixed $value) {}
8+
public function __destruct() {
9+
var_dump(__METHOD__);
10+
}
11+
}
12+
13+
function g() {
14+
yield 'foo';
15+
Fiber::suspend();
16+
}
17+
18+
function f($canary) {
19+
var_dump(yield from g());
20+
}
21+
22+
$canary = new Canary(null);
23+
24+
$iterable = f($canary);
25+
26+
$fiber = new Fiber(function () use ($iterable, $canary) {
27+
var_dump($canary, $iterable->current());
28+
$iterable->next();
29+
var_dump("not executed");
30+
});
31+
32+
$canary->value = $fiber;
33+
34+
$fiber->start();
35+
36+
$iterable->current();
37+
38+
$fiber = $iterable = $canary = null;
39+
40+
gc_collect_cycles();
41+
42+
?>
43+
==DONE==
44+
--EXPECTF--
45+
object(Canary)#%d (1) {
46+
["value"]=>
47+
object(Fiber)#%d (0) {
48+
}
49+
}
50+
string(3) "foo"
51+
string(18) "Canary::__destruct"
52+
==DONE==

‎Zend/tests/gh15330-005.phpt

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
--TEST--
2+
GH-15330 005: Do not scan generator frames more than once
3+
--FILE--
4+
<?php
5+
6+
class Canary {
7+
public function __construct(public mixed $value) {}
8+
public function __destruct() {
9+
var_dump(__METHOD__);
10+
}
11+
}
12+
13+
function g() {
14+
yield 'foo';
15+
Fiber::suspend();
16+
}
17+
18+
function f($canary) {
19+
var_dump(yield from g());
20+
}
21+
22+
$canary = new Canary(null);
23+
24+
$iterable = f($canary);
25+
26+
$fiber = new Fiber(function () use ($iterable, $canary) {
27+
var_dump($canary, $iterable->current());
28+
$f = $iterable->next(...);
29+
$f();
30+
var_dump("not executed");
31+
});
32+
33+
$canary->value = $fiber;
34+
35+
$fiber->start();
36+
37+
$iterable->current();
38+
39+
$fiber = $iterable = $canary = null;
40+
41+
gc_collect_cycles();
42+
43+
?>
44+
==DONE==
45+
--EXPECTF--
46+
object(Canary)#%d (1) {
47+
["value"]=>
48+
object(Fiber)#%d (0) {
49+
}
50+
}
51+
string(3) "foo"
52+
string(18) "Canary::__destruct"
53+
==DONE==

‎Zend/tests/gh15330-006.phpt

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
--TEST--
2+
GH-15330 006: Do not scan generator frames more than once
3+
--FILE--
4+
<?php
5+
6+
class Canary {
7+
public function __construct(public mixed $value) {}
8+
public function __destruct() {
9+
var_dump(__METHOD__);
10+
}
11+
}
12+
13+
function h() {
14+
yield 'foo';
15+
Fiber::suspend();
16+
}
17+
18+
function g() {
19+
yield from h();
20+
}
21+
22+
function f($canary) {
23+
var_dump(yield from g());
24+
}
25+
26+
$canary = new Canary(null);
27+
28+
$iterable = f($canary);
29+
30+
$fiber = new Fiber(function () use ($iterable, $canary) {
31+
var_dump($canary, $iterable->current());
32+
$iterable->next();
33+
var_dump("not executed");
34+
});
35+
36+
$canary->value = $fiber;
37+
38+
$fiber->start();
39+
40+
$iterable->current();
41+
42+
$fiber = $iterable = $canary = null;
43+
44+
gc_collect_cycles();
45+
46+
?>
47+
==DONE==
48+
--EXPECTF--
49+
object(Canary)#%d (1) {
50+
["value"]=>
51+
object(Fiber)#%d (0) {
52+
}
53+
}
54+
string(3) "foo"
55+
string(18) "Canary::__destruct"
56+
==DONE==

‎Zend/zend_execute.c

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4437,7 +4437,20 @@ ZEND_API ZEND_ATTRIBUTE_DEPRECATED HashTable *zend_unfinished_execution_gc(zend_
44374437

44384438
ZEND_API HashTable *zend_unfinished_execution_gc_ex(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer, bool suspended_by_yield)
44394439
{
4440-
if (!EX(func) || !ZEND_USER_CODE(EX(func)->common.type)) {
4440+
if (!EX(func)) {
4441+
return NULL;
4442+
}
4443+
4444+
if (EX_CALL_INFO() & ZEND_CALL_RELEASE_THIS) {
4445+
zend_get_gc_buffer_add_obj(gc_buffer, Z_OBJ(execute_data->This));
4446+
}
4447+
4448+
if (EX_CALL_INFO() & ZEND_CALL_CLOSURE) {
4449+
zend_get_gc_buffer_add_obj(gc_buffer, ZEND_CLOSURE_OBJECT(EX(func)));
4450+
}
4451+
4452+
if (!ZEND_USER_CODE(EX(func)->common.type)) {
4453+
ZEND_ASSERT(!(EX_CALL_INFO() & (ZEND_CALL_HAS_SYMBOL_TABLE|ZEND_CALL_FREE_EXTRA_ARGS|ZEND_CALL_HAS_EXTRA_NAMED_PARAMS)));
44414454
return NULL;
44424455
}
44434456

@@ -4458,12 +4471,6 @@ ZEND_API HashTable *zend_unfinished_execution_gc_ex(zend_execute_data *execute_d
44584471
}
44594472
}
44604473

4461-
if (EX_CALL_INFO() & ZEND_CALL_RELEASE_THIS) {
4462-
zend_get_gc_buffer_add_obj(gc_buffer, Z_OBJ(execute_data->This));
4463-
}
4464-
if (EX_CALL_INFO() & ZEND_CALL_CLOSURE) {
4465-
zend_get_gc_buffer_add_obj(gc_buffer, ZEND_CLOSURE_OBJECT(EX(func)));
4466-
}
44674474
if (EX_CALL_INFO() & ZEND_CALL_HAS_EXTRA_NAMED_PARAMS) {
44684475
zval extra_named_params;
44694476
ZVAL_ARR(&extra_named_params, EX(extra_named_params));

‎Zend/zend_fibers.c

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include "zend.h"
2121
#include "zend_API.h"
22+
#include "zend_gc.h"
2223
#include "zend_ini.h"
2324
#include "zend_vm.h"
2425
#include "zend_exceptions.h"
@@ -27,6 +28,7 @@
2728
#include "zend_mmap.h"
2829
#include "zend_compile.h"
2930
#include "zend_closures.h"
31+
#include "zend_generators.h"
3032

3133
#include "zend_fibers.h"
3234
#include "zend_fibers_arginfo.h"
@@ -682,7 +684,25 @@ static HashTable *zend_fiber_object_gc(zend_object *object, zval **table, int *n
682684
HashTable *lastSymTable = NULL;
683685
zend_execute_data *ex = fiber->execute_data;
684686
for (; ex; ex = ex->prev_execute_data) {
685-
HashTable *symTable = zend_unfinished_execution_gc_ex(ex, ex->func && ZEND_USER_CODE(ex->func->type) ? ex->call : NULL, buf, false);
687+
HashTable *symTable;
688+
if (ZEND_CALL_INFO(ex) & ZEND_CALL_GENERATOR) {
689+
/* The generator object is stored in ex->return_value */
690+
zend_generator *generator = (zend_generator*)ex->return_value;
691+
/* There are two cases to consider:
692+
* - If the generator is currently running, the Generator's GC
693+
* handler will ignore it because it is not collectable. However,
694+
* in this context the generator is suspended in Fiber::suspend()
695+
* and may be collectable, so we can inspect it.
696+
* - If the generator is not running, the Generator's GC handler
697+
* will inspect it. In this case we have to skip the frame.
698+
*/
699+
if (!(generator->flags & ZEND_GENERATOR_CURRENTLY_RUNNING)) {
700+
continue;
701+
}
702+
symTable = zend_generator_frame_gc(buf, generator);
703+
} else {
704+
symTable = zend_unfinished_execution_gc_ex(ex, ex->func && ZEND_USER_CODE(ex->func->type) ? ex->call : NULL, buf, false);
705+
}
686706
if (symTable) {
687707
if (lastSymTable) {
688708
zval *val;

0 commit comments

Comments
 (0)