1
0
mirror of https://github.com/danog/amp.git synced 2024-11-26 20:15:00 +01:00

Fix exceptions bubbling from Coroutine::__destruct

This has been an edge case potentially hiding some exceptions. The tests have been refactored to error if the loop has watchers leaking from one test to another test.
This commit is contained in:
Niklas Keller 2019-05-14 21:37:08 +02:00 committed by Aaron Piotrowski
parent aeb5de16d9
commit c12828081f
37 changed files with 242 additions and 175 deletions

View File

@ -36,6 +36,7 @@
"php": ">=7"
},
"require-dev": {
"ext-json": "*",
"amphp/phpunit-util": "^1",
"amphp/php-cs-fixer-config": "dev-master",
"react/promise": "^2",

View File

@ -15,103 +15,16 @@ final class Coroutine implements Promise
{
use Internal\Placeholder;
/** @var \Generator */
private $generator;
/** @var callable(\Throwable|null $exception, mixed $value): void */
private $onResolve;
/** @var bool Used to control iterative coroutine continuation. */
private $immediate = true;
/** @var \Throwable|null Promise failure reason when executing next coroutine step, null at all other times. */
private $exception;
/** @var mixed Promise success value when executing next coroutine step, null at all other times. */
private $value;
/**
* @param \Generator $generator
*/
public function __construct(\Generator $generator)
{
$this->generator = $generator;
try {
$yielded = $this->generator->current();
if (!$yielded instanceof Promise) {
if (!$this->generator->valid()) {
$this->resolve($this->generator->getReturn());
return;
}
$yielded = $this->transform($yielded);
}
} catch (\Throwable $exception) {
$this->fail($exception);
return;
}
/**
* @param \Throwable|null $exception Exception to be thrown into the generator.
* @param mixed $value Value to be sent into the generator.
*/
$this->onResolve = function ($exception, $value) {
$this->exception = $exception;
$this->value = $value;
if (!$this->immediate) {
$this->immediate = true;
return;
}
try {
do {
if ($this->exception) {
// Throw exception at current execution point.
$yielded = $this->generator->throw($this->exception);
} else {
// Send the new value and execute to next yield statement.
$yielded = $this->generator->send($this->value);
}
if (!$yielded instanceof Promise) {
if (!$this->generator->valid()) {
$this->resolve($this->generator->getReturn());
$this->onResolve = null;
return;
}
$yielded = $this->transform($yielded);
}
$this->immediate = false;
$yielded->onResolve($this->onResolve);
} while ($this->immediate);
$this->immediate = true;
} catch (\Throwable $exception) {
$this->fail($exception);
$this->onResolve = null;
} finally {
$this->exception = null;
$this->value = null;
}
};
$yielded->onResolve($this->onResolve);
}
/**
* Attempts to transform the non-promise yielded from the generator into a promise, otherwise returns an instance
* `Amp\Failure` failed with an instance of `Amp\InvalidYieldError`.
*
* @param mixed $yielded Non-promise yielded from generator.
* @param mixed $yielded Non-promise yielded from generator.
* @param \Generator $generator No type for performance, we already know the type.
*
* @return \Amp\Promise
* @return Promise
*/
private function transform($yielded): Promise
private static function transform($yielded, $generator): Promise
{
try {
if (\is_array($yielded)) {
@ -128,7 +41,7 @@ final class Coroutine implements Promise
}
return new Failure(new InvalidYieldError(
$this->generator,
$generator,
\sprintf(
"Unexpected yield; Expected an instance of %s or %s or an array of such instances",
Promise::class,
@ -137,4 +50,115 @@ final class Coroutine implements Promise
$exception ?? null
));
}
/**
* @param \Generator $generator
*/
public function __construct(\Generator $generator)
{
try {
$yielded = $generator->current();
if (!$yielded instanceof Promise) {
if (!$generator->valid()) {
$this->resolve($generator->getReturn());
return;
}
$yielded = self::transform($yielded, $generator);
}
} catch (\Throwable $exception) {
$this->fail($exception);
return;
}
/** @var bool Used to control iterative coroutine continuation. */
$immediate = true;
/** @var \Throwable|null Promise failure reason when executing next coroutine step, null at all other times. */
$exception = null;
/** @var mixed Promise success value when executing next coroutine step, null at all other times. */
$value = null;
/**
* @param \Throwable|null $e Exception to be thrown into the generator.
* @param mixed $v Value to be sent into the generator.
*/
$onResolve = function ($e, $v) use ($generator, &$exception, &$value, &$immediate, &$onResolve) {
$exception = $e;
$value = $v;
if (!$immediate) {
$immediate = true;
return;
}
try {
do {
if ($exception) {
// Throw exception at current execution point.
$yielded = $generator->throw($exception);
} else {
// Send the new value and execute to next yield statement.
$yielded = $generator->send($value);
}
if (!$yielded instanceof Promise) {
if (!$generator->valid()) {
$this->resolve($generator->getReturn());
$onResolve = null;
return;
}
$yielded = self::transform($yielded, $generator);
}
$immediate = false;
$yielded->onResolve($onResolve);
} while ($immediate);
$immediate = true;
} catch (\Throwable $exception) {
try {
$this->fail($exception);
$onResolve = null;
} catch (\Throwable $e) {
Loop::defer(static function () use ($e) {
throw $e;
});
}
} finally {
try {
$exception = null;
$value = null;
} catch (\Throwable $e) {
Loop::defer(static function () use ($e) {
throw $e;
});
}
}
};
try {
$yielded->onResolve($onResolve);
unset($generator, $yielded, $exception, $value, $immediate, $onResolve);
} catch (\Throwable $e) {
Loop::defer(static function () use ($e) {
throw $e;
});
}
}
public function __destruct()
{
try {
$this->result = null;
} catch (\Throwable $e) {
Loop::defer(static function () use ($e) {
throw $e;
});
}
}
}

View File

@ -22,7 +22,7 @@ trait Placeholder
/** @var mixed */
private $result;
/** @var callable|\Amp\Internal\ResolutionQueue|null */
/** @var callable|ResolutionQueue|null */
private $onResolved;
/** @var null|array */
@ -128,6 +128,7 @@ trait Placeholder
try {
$result = $onResolved(null, $this->result);
$onResolved = null; // allow garbage collection of $onResolved, to catch any exceptions from destructors
if ($result === null) {
return;

View File

@ -33,7 +33,7 @@ class PromiseMock
}
}
class AdaptTest extends \PHPUnit\Framework\TestCase
class AdaptTest extends BaseTest
{
public function testThenCalled()
{

View File

@ -6,10 +6,9 @@ use Amp\Delayed;
use Amp\Loop;
use Amp\Promise;
use Amp\Success;
use PHPUnit\Framework\TestCase;
use React\Promise\FulfilledPromise;
class AllTest extends TestCase
class AllTest extends BaseTest
{
public function testEmptyArray()
{

View File

@ -8,7 +8,7 @@ use Amp\Loop;
use Amp\Promise;
use Amp\Success;
class AnyTest extends \PHPUnit\Framework\TestCase
class AnyTest extends BaseTest
{
public function testEmptyArray()
{

45
test/BaseTest.php Normal file
View File

@ -0,0 +1,45 @@
<?php
namespace Amp\Test;
use Amp\Delayed;
use Amp\Loop;
use Amp\PHPUnit\TestCase;
use function Amp\Promise\wait;
abstract class BaseTest extends TestCase
{
public function tearDown()
{
parent::tearDown();
Loop::setErrorHandler();
$this->clearLoopRethrows();
}
private function clearLoopRethrows()
{
$errors = [];
retry:
try {
wait(new Delayed(0));
} catch (\Error $e) {
$errors[] = (string) $e;
goto retry;
}
if ($errors) {
\set_error_handler(null);
\trigger_error(\implode("\n", $errors), E_USER_ERROR);
}
$info = Loop::getInfo();
if ($info['enabled_watchers']['referenced'] + $info['enabled_watchers']['unreferenced'] > 0) {
\set_error_handler(null);
\trigger_error("Found enabled watchers on test end: " . \json_encode($info, \JSON_PRETTY_PRINT), E_USER_ERROR);
}
}
}

View File

@ -9,10 +9,9 @@ use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Promise;
use Amp\Success;
use PHPUnit\Framework\TestCase;
use React\Promise\FulfilledPromise as FulfilledReactPromise;
class CallTest extends TestCase
class CallTest extends BaseTest
{
public function testCallWithFunctionReturningPromise()
{

View File

@ -20,7 +20,7 @@ class CallableMaker
}
}
class CallableMakerTest extends \PHPUnit\Framework\TestCase
class CallableMakerTest extends BaseTest
{
/** @var \Amp\Test\CallableMaker */
private $maker;

View File

@ -6,12 +6,11 @@ use Amp\CancellationToken;
use Amp\CancellationTokenSource;
use Amp\Emitter;
use Amp\Loop;
use Amp\PHPUnit\TestCase;
use Amp\PHPUnit\TestException;
use Amp\Success;
use function Amp\asyncCall;
class CancellationTest extends TestCase
class CancellationTest extends BaseTest
{
private function createAsyncIterator(CancellationToken $cancellationToken)
{

View File

@ -7,7 +7,7 @@ use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Producer;
class ConcatTest extends \PHPUnit\Framework\TestCase
class ConcatTest extends BaseTest
{
public function getArrays()
{

View File

@ -10,12 +10,11 @@ use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Promise;
use Amp\Success;
use PHPUnit\Framework\TestCase;
use React\Promise\FulfilledPromise as FulfilledReactPromise;
use React\Promise\Promise as ReactPromise;
use function Amp\call;
class CoroutineTest extends TestCase
class CoroutineTest extends BaseTest
{
const TIMEOUT = 100;
@ -505,7 +504,23 @@ class CoroutineTest extends TestCase
});
};
new Coroutine($generator());
try {
new Coroutine($generator());
} catch (\Throwable $e) {
$this->fail("Caught exception that shouldn't be thrown at that place.");
}
$this->expectExceptionObject($exception);
try {
Promise\wait(new Delayed(0)); // make loop tick once to throw errors from loop
} catch (\Error $e) {
if ($e->getMessage() === "Loop exceptionally stopped without resolving the promise") {
throw $e->getPrevious();
}
throw $e;
}
}
/**

View File

@ -5,7 +5,7 @@ namespace Amp\Test;
use Amp\Deferred;
use Amp\Promise;
class DeferredTest extends \PHPUnit\Framework\TestCase
class DeferredTest extends BaseTest
{
/** @var \Amp\Deferred */
private $deferred;

View File

@ -4,8 +4,9 @@ namespace Amp\Test;
use Amp\Delayed;
use Amp\Loop;
use function Amp\Promise\wait;
class DelayedTest extends \PHPUnit\Framework\TestCase
class DelayedTest extends BaseTest
{
public function testDelayed()
{
@ -13,10 +14,10 @@ class DelayedTest extends \PHPUnit\Framework\TestCase
$value = "test";
$start = \microtime(true);
Loop::run(function () use (&$result, $time, $value) {
Loop::run(static function () use (&$result, $time, $value) {
$promise = new Delayed($time, $value);
$callback = function ($exception, $value) use (&$result) {
$callback = static function ($exception, $value) use (&$result) {
$result = $value;
};
@ -34,11 +35,11 @@ class DelayedTest extends \PHPUnit\Framework\TestCase
$start = \microtime(true);
$invoked = false;
Loop::run(function () use (&$invoked, $time, $value) {
Loop::run(static function () use (&$invoked, $time, $value, &$promise) {
$promise = new Delayed($time, $value);
$promise->unreference();
$callback = function ($exception, $value) use (&$invoked) {
$callback = static function () use (&$invoked) {
$invoked = true;
};
@ -47,6 +48,10 @@ class DelayedTest extends \PHPUnit\Framework\TestCase
$this->assertLessThanOrEqual($time - 1 /* 1ms grace period */, (\microtime(true) - $start) * 1000);
$this->assertFalse($invoked);
// clear watcher
$promise->reference();
wait($promise);
}
/**
@ -59,12 +64,12 @@ class DelayedTest extends \PHPUnit\Framework\TestCase
$start = \microtime(true);
$invoked = false;
Loop::run(function () use (&$invoked, $time, $value) {
Loop::run(static function () use (&$invoked, $time, $value) {
$promise = new Delayed($time, $value);
$promise->unreference();
$promise->reference();
$callback = function ($exception, $value) use (&$invoked) {
$callback = static function ($exception, $value) use (&$invoked) {
$invoked = true;
};

View File

@ -6,7 +6,7 @@ use Amp\Failure;
use Amp\Loop;
use React\Promise\RejectedPromise as RejectedReactPromise;
class FailureTest extends \PHPUnit\Framework\TestCase
class FailureTest extends BaseTest
{
/**
* @expectedException \TypeError

View File

@ -7,9 +7,8 @@ use Amp\Iterator;
use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Producer;
use PHPUnit\Framework\TestCase;
class FilterTest extends TestCase
class FilterTest extends BaseTest
{
public function testNoValuesEmitted()
{

View File

@ -10,7 +10,7 @@ use Amp\Promise;
use Amp\Success;
use React\Promise\FulfilledPromise;
class FirstTest extends \PHPUnit\Framework\TestCase
class FirstTest extends BaseTest
{
/**
* @expectedException \Error

View File

@ -4,7 +4,7 @@ namespace Amp\Test;
use Amp\InvalidYieldError;
class InvalidYieldErrorTest extends \PHPUnit\Framework\TestCase
class InvalidYieldErrorTest extends BaseTest
{
public function testWithInvalidGenerator()
{

View File

@ -9,7 +9,7 @@ use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Success;
class IteratorFromIterableTest extends \PHPUnit\Framework\TestCase
class IteratorFromIterableTest extends BaseTest
{
const TIMEOUT = 10;

View File

@ -3,10 +3,9 @@
namespace Amp\Test;
use Amp\Iterator;
use Amp\PHPUnit\TestCase;
use function Amp\Promise\wait;
class IteratorToArrayTest extends TestCase
class IteratorToArrayTest extends BaseTest
{
public function testNonEmpty()
{

View File

@ -5,11 +5,10 @@ namespace Amp\Test;
use Amp\Failure;
use Amp\LazyPromise;
use Amp\Success;
use PHPUnit\Framework\TestCase;
use React\Promise\FulfilledPromise as FulfilledReactPromise;
use React\Promise\RejectedPromise as RejectedReactPromise;
class LazyPromiseTest extends TestCase
class LazyPromiseTest extends BaseTest
{
public function testPromisorNotCalledOnConstruct()
{

View File

@ -3,9 +3,8 @@
namespace Amp\Test;
use Amp\Loop;
use PHPUnit\Framework\TestCase;
class LoopTest extends TestCase
class LoopTest extends BaseTest
{
public function testDelayWithNegativeDelay()
{
@ -29,8 +28,9 @@ class LoopTest extends TestCase
$ends = \stream_socket_pair(\stripos(PHP_OS, "win") === 0 ? STREAM_PF_INET : STREAM_PF_UNIX, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP);
\fwrite($ends[0], "trigger readability watcher");
Loop::onReadable($ends[1], function () {
Loop::onReadable($ends[1], function ($watcher) {
$this->assertTrue(true);
Loop::cancel($watcher);
Loop::stop();
});
});
@ -39,8 +39,9 @@ class LoopTest extends TestCase
public function testOnWritable()
{
Loop::run(function () {
Loop::onWritable(STDOUT, function () {
Loop::onWritable(STDOUT, function ($watcher) {
$this->assertTrue(true);
Loop::cancel($watcher);
Loop::stop();
});
});

View File

@ -8,7 +8,7 @@ use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Producer;
class MapTest extends \PHPUnit\Framework\TestCase
class MapTest extends BaseTest
{
public function testNoValuesEmitted()
{

View File

@ -7,9 +7,8 @@ use Amp\Iterator;
use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Producer;
use PHPUnit\Framework\TestCase;
class MergeTest extends TestCase
class MergeTest extends BaseTest
{
public function getArrays()
{

View File

@ -13,9 +13,9 @@ class Placeholder
}
}
class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
class PlaceholderTraitTest extends BaseTest
{
/** @var \Amp\Test\Placeholder */
/** @var Placeholder */
private $placeholder;
public function setUp()
@ -29,8 +29,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $value;
++$invoked;
};
$this->placeholder->onResolve($callback);
@ -50,8 +50,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $value;
++$invoked;
};
$this->placeholder->onResolve($callback);
@ -73,8 +73,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $value;
++$invoked;
};
$this->placeholder->resolve($value);
@ -94,8 +94,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $value;
++$invoked;
};
$this->placeholder->resolve($value);
@ -118,8 +118,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$expected = new \Exception;
Loop::setErrorHandler(function ($exception) use (&$invoked, $expected) {
++$invoked;
$this->assertSame($expected, $exception);
++$invoked;
});
$callback = function () use ($expected) {
@ -144,8 +144,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$expected = new \Exception;
Loop::setErrorHandler(function ($exception) use (&$invoked, $expected) {
++$invoked;
$this->assertSame($expected, $exception);
++$invoked;
});
$callback = function () use ($expected) {
@ -166,8 +166,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $exception;
++$invoked;
};
$this->placeholder->onResolve($callback);
@ -187,8 +187,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $exception;
++$invoked;
};
$this->placeholder->onResolve($callback);
@ -210,8 +210,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $exception;
++$invoked;
};
$this->placeholder->fail($exception);
@ -231,8 +231,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$invoked = 0;
$callback = function ($exception, $value) use (&$invoked, &$result) {
++$invoked;
$result = $exception;
++$invoked;
};
$this->placeholder->fail($exception);
@ -255,8 +255,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$expected = new \Exception;
Loop::setErrorHandler(function ($exception) use (&$invoked, $expected) {
++$invoked;
$this->assertSame($expected, $exception);
++$invoked;
});
$callback = function () use ($expected) {
@ -281,8 +281,8 @@ class PlaceholderTraitTest extends \PHPUnit\Framework\TestCase
$expected = new \Exception;
Loop::setErrorHandler(function ($exception) use (&$invoked, $expected) {
++$invoked;
$this->assertSame($expected, $exception);
++$invoked;
});
$callback = function () use ($expected) {

View File

@ -7,9 +7,8 @@ use Amp\Delayed;
use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Producer;
use PHPUnit\Framework\TestCase;
class ProducerTest extends TestCase
class ProducerTest extends BaseTest
{
const TIMEOUT = 100;

View File

@ -8,7 +8,6 @@ use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Promise;
use Amp\Success;
use PHPUnit\Framework\TestCase;
use React\Promise\FulfilledPromise as FulfilledReactPromise;
class Producer
@ -20,7 +19,7 @@ class Producer
}
}
class ProducerTraitTest extends TestCase
class ProducerTraitTest extends BaseTest
{
/** @var \Amp\Test\Producer */
private $producer;

View File

@ -13,7 +13,7 @@ class Promise implements \Amp\Promise
}
}
class PromiseTest extends \PHPUnit\Framework\TestCase
class PromiseTest extends BaseTest
{
private $originalErrorHandler;
@ -36,18 +36,6 @@ class PromiseTest extends \PHPUnit\Framework\TestCase
];
}
public function setUp()
{
$this->originalErrorHandler = Loop::setErrorHandler(function ($e) {
throw $e;
});
}
public function tearDown()
{
Loop::setErrorHandler($this->originalErrorHandler);
}
public function provideSuccessValues()
{
return [

View File

@ -5,10 +5,9 @@ namespace Amp\Test;
use Amp\Failure;
use Amp\Loop;
use Amp\Promise;
use PHPUnit\Framework\TestCase;
use function React\Promise\reject;
class RethrowTest extends TestCase
class RethrowTest extends BaseTest
{
public function testRethrow()
{

View File

@ -8,10 +8,9 @@ use Amp\Loop;
use Amp\MultiReasonException;
use Amp\Promise;
use Amp\Success;
use PHPUnit\Framework\TestCase;
use React\Promise\FulfilledPromise;
class SomeTest extends TestCase
class SomeTest extends BaseTest
{
public function testEmptyArray()
{

View File

@ -9,7 +9,7 @@ class StructTestFixture
public $_foofoofoofoofoofoofoofoobar;
}
class StructTest extends \PHPUnit\Framework\TestCase
class StructTest extends BaseTest
{
/**
* @expectedException \Error

View File

@ -7,7 +7,7 @@ use Amp\Promise;
use Amp\Success;
use React\Promise\RejectedPromise as RejectedReactPromise;
class SuccessTest extends \PHPUnit\Framework\TestCase
class SuccessTest extends BaseTest
{
/**
* @expectedException \Error

View File

@ -5,11 +5,10 @@ namespace Amp\Test;
use Amp\CancelledException;
use Amp\Delayed;
use Amp\Loop;
use Amp\PHPUnit\TestCase;
use Amp\TimeoutCancellationToken;
use Amp\TimeoutException;
class TimeoutCancellationTokenTest extends TestCase
class TimeoutCancellationTokenTest extends BaseTest
{
public function testTimeout()
{

View File

@ -9,7 +9,7 @@ use Amp\Promise;
use Amp\Success;
use function React\Promise\resolve;
class TimeoutTest extends \PHPUnit\Framework\TestCase
class TimeoutTest extends BaseTest
{
public function testSuccessfulPromise()
{

View File

@ -9,7 +9,7 @@ use Amp\Promise;
use Amp\Success;
use function React\Promise\resolve;
class TimeoutWithDefaultTest extends \PHPUnit\Framework\TestCase
class TimeoutWithDefaultTest extends BaseTest
{
public function testSuccessfulPromise()
{

View File

@ -9,10 +9,9 @@ use Amp\Loop;
use Amp\PHPUnit\TestException;
use Amp\Promise;
use Amp\Success;
use PHPUnit\Framework\TestCase;
use function React\Promise\resolve;
class WaitTest extends TestCase
class WaitTest extends BaseTest
{
public function testWaitOnSuccessfulPromise()
{

View File

@ -5,7 +5,7 @@ namespace Amp\Test;
use Amp\Deferred;
use Amp\Promise;
class WrapTest extends \PHPUnit\Framework\TestCase
class WrapTest extends BaseTest
{
public function testSuccess()
{