From cfb1d27026927487d23d8866df64a1ce8224faeb Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Fri, 27 Jan 2017 11:28:05 +0100 Subject: [PATCH] Fix memory leak in react adapter wait Also remove dead code and add react promise composer suggest. --- .../Promise/Adapter/ReactPromiseAdapter.php | 31 +++++--- .../AuthorizationExpressionProvider.php | 2 - .../Adapter/ReactPromiseAdapterTest.php | 72 +++++++++++++++++++ composer.json | 3 +- 4 files changed, 94 insertions(+), 14 deletions(-) create mode 100644 Tests/Executor/Promise/Adapter/ReactPromiseAdapterTest.php diff --git a/Executor/Promise/Adapter/ReactPromiseAdapter.php b/Executor/Promise/Adapter/ReactPromiseAdapter.php index b0357b512..47c51668f 100644 --- a/Executor/Promise/Adapter/ReactPromiseAdapter.php +++ b/Executor/Promise/Adapter/ReactPromiseAdapter.php @@ -41,16 +41,17 @@ public function convertThenable($thenable) /** * Synchronously wait when promise completes. * - * @param Promise $promise + * @param Promise $promise + * @param callable $onProgress * * @return ExecutionResult * * @throws \Exception */ - public function wait(Promise $promise) + public function wait(Promise $promise, callable $onProgress = null) { if (!$this->isThenable($promise)) { - return; + throw new \InvalidArgumentException(sprintf('The "%s" method must be call with compatible a Promise.', __METHOD__)); } $wait = true; $resolvedValue = null; @@ -58,17 +59,25 @@ public function wait(Promise $promise) /** @var \React\Promise\PromiseInterface $reactPromise */ $reactPromise = $promise->adoptedPromise; + $reactPromise->then(function ($values) use (&$resolvedValue, &$wait) { + $resolvedValue = $values; + $wait = false; + }, function ($reason) use (&$exception, &$wait) { + $exception = $reason; + $wait = false; + }); + + // wait until promise resolution while ($wait) { - $reactPromise->then(function ($values) use (&$resolvedValue, &$wait) { - $resolvedValue = $values; - $wait = false; - }, function ($reason) use (&$exception, &$wait) { - $exception = $reason; - $wait = false; - }); + if (null !== $onProgress) { + $onProgress(); + } + // less CPU intensive without sacrificing the performance + usleep(5); } - if ($exception instanceof \Exception) { + /** @var \Exception|null $exception */ + if (null !== $exception) { throw $exception; } diff --git a/ExpressionLanguage/AuthorizationExpressionProvider.php b/ExpressionLanguage/AuthorizationExpressionProvider.php index 43cc7066b..4a68ed12b 100644 --- a/ExpressionLanguage/AuthorizationExpressionProvider.php +++ b/ExpressionLanguage/AuthorizationExpressionProvider.php @@ -52,8 +52,6 @@ function () { 'isFullyAuthenticated', function () { return '$container->get(\'security.authorization_checker\')->isGranted(\'IS_AUTHENTICATED_FULLY\')'; - }, - function () { } ), diff --git a/Tests/Executor/Promise/Adapter/ReactPromiseAdapterTest.php b/Tests/Executor/Promise/Adapter/ReactPromiseAdapterTest.php new file mode 100644 index 000000000..1f0276a21 --- /dev/null +++ b/Tests/Executor/Promise/Adapter/ReactPromiseAdapterTest.php @@ -0,0 +1,72 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Overblog\GraphQLBundle\Tests\Error; + +use GraphQL\Executor\Promise\Promise; +use Overblog\GraphQLBundle\Executor\Promise\Adapter\ReactPromiseAdapter; +use Symfony\Component\Process\PhpProcess; + +class ReactPromiseAdapterTest extends \PHPUnit_Framework_TestCase +{ + /** @var ReactPromiseAdapter */ + private $adapter; + + public function setUp() + { + $this->adapter = new ReactPromiseAdapter(); + } + + /** + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage The "Overblog\GraphQLBundle\Executor\Promise\Adapter\ReactPromiseAdapter::wait" method must be call with compatible a Promise. + */ + public function testWaitWithNotSupportedPromise() + { + $noSupportedPromise = new Promise(new \stdClass(), $this->adapter); + $this->adapter->wait($noSupportedPromise); + } + + /** + * @expectedException \Exception + * @expectedExceptionMessage Promise has been rejected! + */ + public function testWaitRejectedPromise() + { + $rejected = $this->adapter->createRejected(new \Exception('Promise has been rejected!')); + $this->adapter->wait($rejected); + } + + public function testWaitAsyncPromise() + { + $output = 'OK!'; + $process = new PhpProcess(<<adapter->create(function (callable $resolve) use (&$process) { + $process->start(function () use ($resolve, &$process) { + $output = $process->getOutput(); + $resolve($output); + }); + }); + + $this->assertEquals( + $output, + $this->adapter->wait($promise, function () use (&$process) { + $process->wait(); + }) + ); + } +} diff --git a/composer.json b/composer.json index 39d2b79ac..e8e4d5bc4 100644 --- a/composer.json +++ b/composer.json @@ -39,7 +39,8 @@ "webonyx/graphql-php": "^0.9.0" }, "suggest": { - "twig/twig": "If you want to use graphiQL." + "twig/twig": "If you want to use graphiQL.", + "react/promise": "To use ReactPHP promise adapter" }, "require-dev": { "friendsofphp/php-cs-fixer": "^1.11",