From d460d70b16eeb1a1810239675227260460b10860 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 19 Nov 2021 07:03:45 +0100 Subject: [PATCH 1/3] Calling `cancel()` on coroutine should cancel pending promise --- src/functions.php | 10 ++++++++-- tests/CoroutineTest.php | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/functions.php b/src/functions.php index f60b6b9..2d1c12f 100644 --- a/src/functions.php +++ b/src/functions.php @@ -224,10 +224,16 @@ function coroutine(callable $function, ...$args): PromiseInterface return resolve($generator); } - $deferred = new Deferred(); + $promise = null; + $deferred = new Deferred(function () use (&$promise) { + if ($promise instanceof CancellablePromiseInterface) { + $promise->cancel(); + } + $promise = null; + }); /** @var callable $next */ - $next = function () use ($deferred, $generator, &$next) { + $next = function () use ($deferred, $generator, &$next, &$promise) { try { if (!$generator->valid()) { $deferred->resolve($generator->getReturn()); diff --git a/tests/CoroutineTest.php b/tests/CoroutineTest.php index 97dbe38..9107e39 100644 --- a/tests/CoroutineTest.php +++ b/tests/CoroutineTest.php @@ -2,6 +2,7 @@ namespace React\Tests\Async; +use React\Promise\Promise; use function React\Async\coroutine; use function React\Promise\reject; use function React\Promise\resolve; @@ -104,4 +105,17 @@ public function testCoroutineReturnsRejectedPromiseIfFunctionYieldsInvalidValue( $promise->then(null, $this->expectCallableOnceWith(new \UnexpectedValueException('Expected coroutine to yield React\Promise\PromiseInterface, but got integer'))); } + + public function testCoroutineWillCancelPendingPromiseWhenCallingCancelOnResultingPromise() + { + $promise = coroutine(function () { + yield new Promise(function () { }, function () { + throw new \RuntimeException('Operation cancelled', 42); + }); + }); + + $promise->cancel(); + + $promise->then(null, $this->expectCallableOnceWith(new \RuntimeException('Operation cancelled', 42))); + } } From 603e70bfce9508836e781a9f45284e2ef527c96f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 19 Nov 2021 07:20:56 +0100 Subject: [PATCH 2/3] Improve cancellation for coroutines continuing to yield pending promises --- src/functions.php | 8 +++++--- tests/CoroutineTest.php | 29 +++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/functions.php b/src/functions.php index 2d1c12f..7e9cb93 100644 --- a/src/functions.php +++ b/src/functions.php @@ -226,10 +226,12 @@ function coroutine(callable $function, ...$args): PromiseInterface $promise = null; $deferred = new Deferred(function () use (&$promise) { - if ($promise instanceof CancellablePromiseInterface) { - $promise->cancel(); + // cancel pending promise(s) as long as generator function keeps yielding + while ($promise instanceof CancellablePromiseInterface) { + $temp = $promise; + $promise = null; + $temp->cancel(); } - $promise = null; }); /** @var callable $next */ diff --git a/tests/CoroutineTest.php b/tests/CoroutineTest.php index 9107e39..d3c8209 100644 --- a/tests/CoroutineTest.php +++ b/tests/CoroutineTest.php @@ -106,16 +106,41 @@ public function testCoroutineReturnsRejectedPromiseIfFunctionYieldsInvalidValue( $promise->then(null, $this->expectCallableOnceWith(new \UnexpectedValueException('Expected coroutine to yield React\Promise\PromiseInterface, but got integer'))); } + public function testCoroutineWillCancelPendingPromiseWhenCallingCancelOnResultingPromise() + { + $cancelled = 0; + $promise = coroutine(function () use (&$cancelled) { + yield new Promise(function () use (&$cancelled) { + ++$cancelled; + }); + }); + + $promise->cancel(); + + $this->assertEquals(1, $cancelled); + } + + public function testCoroutineWillCancelAllPendingPromisesWhenFunctionContinuesToYieldWhenCallingCancelOnResultingPromise() { $promise = coroutine(function () { + $promise = new Promise(function () { }, function () { + throw new \RuntimeException('Frist operation cancelled', 21); + }); + + try { + yield $promise; + } catch (\RuntimeException $e) { + // ignore exception and continue + } + yield new Promise(function () { }, function () { - throw new \RuntimeException('Operation cancelled', 42); + throw new \RuntimeException('Second operation cancelled', 42); }); }); $promise->cancel(); - $promise->then(null, $this->expectCallableOnceWith(new \RuntimeException('Operation cancelled', 42))); + $promise->then(null, $this->expectCallableOnceWith(new \RuntimeException('Second operation cancelled', 42))); } } From 4541391beb4ed5b1433870a9599836dda233ea8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 19 Nov 2021 07:38:49 +0100 Subject: [PATCH 3/3] Clean up garbage references for coroutines --- src/functions.php | 6 ++- tests/CoroutineTest.php | 95 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/functions.php b/src/functions.php index 7e9cb93..ad91688 100644 --- a/src/functions.php +++ b/src/functions.php @@ -238,16 +238,19 @@ function coroutine(callable $function, ...$args): PromiseInterface $next = function () use ($deferred, $generator, &$next, &$promise) { try { if (!$generator->valid()) { + $next = null; $deferred->resolve($generator->getReturn()); return; } } catch (\Throwable $e) { + $next = null; $deferred->reject($e); return; } $promise = $generator->current(); if (!$promise instanceof PromiseInterface) { + $next = null; $deferred->reject(new \UnexpectedValueException( 'Expected coroutine to yield ' . PromiseInterface::class . ', but got ' . (is_object($promise) ? get_class($promise) : gettype($promise)) )); @@ -260,7 +263,8 @@ function coroutine(callable $function, ...$args): PromiseInterface }, function (\Throwable $reason) use ($generator, $next) { $generator->throw($reason); $next(); - })->then(null, function (\Throwable $reason) use ($deferred) { + })->then(null, function (\Throwable $reason) use ($deferred, &$next) { + $next = null; $deferred->reject($reason); }); }; diff --git a/tests/CoroutineTest.php b/tests/CoroutineTest.php index d3c8209..6e461d5 100644 --- a/tests/CoroutineTest.php +++ b/tests/CoroutineTest.php @@ -143,4 +143,99 @@ public function testCoroutineWillCancelAllPendingPromisesWhenFunctionContinuesTo $promise->then(null, $this->expectCallableOnceWith(new \RuntimeException('Second operation cancelled', 42))); } + + public function testCoroutineShouldNotCreateAnyGarbageReferencesWhenGeneratorReturns() + { + if (class_exists('React\Promise\When')) { + $this->markTestSkipped('Not supported on legacy Promise v1 API'); + } + + gc_collect_cycles(); + gc_collect_cycles(); + + $promise = coroutine(function () { + if (false) { + yield; + } + return 42; + }); + + unset($promise); + + $this->assertEquals(0, gc_collect_cycles()); + } + + public function testCoroutineShouldNotCreateAnyGarbageReferencesForPromiseRejectedWithExceptionImmediately() + { + if (class_exists('React\Promise\When')) { + $this->markTestSkipped('Not supported on legacy Promise v1 API'); + } + + gc_collect_cycles(); + + $promise = coroutine(function () { + yield new Promise(function () { + throw new \RuntimeException('Failed', 42); + }); + }); + + unset($promise); + + $this->assertEquals(0, gc_collect_cycles()); + } + + public function testCoroutineShouldNotCreateAnyGarbageReferencesForPromiseRejectedWithExceptionOnCancellation() + { + if (class_exists('React\Promise\When')) { + $this->markTestSkipped('Not supported on legacy Promise v1 API'); + } + + gc_collect_cycles(); + + $promise = coroutine(function () { + yield new Promise(function () { }, function () { + throw new \RuntimeException('Operation cancelled', 42); + }); + }); + + $promise->cancel(); + unset($promise); + + $this->assertEquals(0, gc_collect_cycles()); + } + + public function testCoroutineShouldNotCreateAnyGarbageReferencesWhenGeneratorThrowsBeforeFirstYield() + { + if (class_exists('React\Promise\When')) { + $this->markTestSkipped('Not supported on legacy Promise v1 API'); + } + + gc_collect_cycles(); + + $promise = coroutine(function () { + throw new \RuntimeException('Failed', 42); + yield; + }); + + unset($promise); + + $this->assertEquals(0, gc_collect_cycles()); + } + + public function testCoroutineShouldNotCreateAnyGarbageReferencesWhenGeneratorYieldsInvalidValue() + { + if (class_exists('React\Promise\When')) { + $this->markTestSkipped('Not supported on legacy Promise v1 API'); + } + + gc_collect_cycles(); + + $promise = coroutine(function () { + yield 42; + }); + + unset($promise); + + $this->assertEquals(0, gc_collect_cycles()); + } }