From a182f194b90c639f34d6bfa097bba1376bae39e6 Mon Sep 17 00:00:00 2001 From: Thomas Vargiu Date: Sun, 16 Feb 2020 02:05:18 +0100 Subject: [PATCH 1/4] Hotfix memory leak recreating new chain instances --- src/PluginChain.php | 60 ++++++++++++++++++++++++++++++++++++++++++++ src/PluginClient.php | 23 +---------------- 2 files changed, 61 insertions(+), 22 deletions(-) create mode 100644 src/PluginChain.php diff --git a/src/PluginChain.php b/src/PluginChain.php new file mode 100644 index 0000000..c6971aa --- /dev/null +++ b/src/PluginChain.php @@ -0,0 +1,60 @@ +plugins = $plugins; + $this->clientCallable = $clientCallable; + $this->maxRestarts = $maxRestarts; + } + + private function createChain(): callable + { + $lastCallable = $this->clientCallable; + + foreach ($this->plugins as $plugin) { + $lastCallable = function (RequestInterface $request) use ($plugin, $lastCallable) { + return $plugin->handleRequest($request, $lastCallable, $this); + }; + } + + return $lastCallable; + } + + public function __invoke(RequestInterface $request): Promise + { + if ($this->restarts > $this->maxRestarts) { + throw new LoopException('Too many restarts in plugin client', $request); + } + + ++$this->restarts; + + return $this->createChain()($request); + } +} diff --git a/src/PluginClient.php b/src/PluginClient.php index f935b0d..67ff020 100644 --- a/src/PluginClient.php +++ b/src/PluginClient.php @@ -125,27 +125,6 @@ private function configure(array $options = []): array */ private function createPluginChain(array $pluginList, callable $clientCallable): callable { - $firstCallable = $lastCallable = $clientCallable; - - while ($plugin = array_pop($pluginList)) { - $lastCallable = function (RequestInterface $request) use ($plugin, $lastCallable, &$firstCallable) { - return $plugin->handleRequest($request, $lastCallable, $firstCallable); - }; - - $firstCallable = $lastCallable; - } - - $firstCalls = 0; - $firstCallable = function (RequestInterface $request) use ($lastCallable, &$firstCalls) { - if ($firstCalls > $this->options['max_restarts']) { - throw new LoopException('Too many restarts in plugin client', $request); - } - - ++$firstCalls; - - return $lastCallable($request); - }; - - return $firstCallable; + return new PluginChain($pluginList, $clientCallable, $this->options['max_restarts']); } } From 93ed1f93a7c686bd7c0893332e560c9a2065da7a Mon Sep 17 00:00:00 2001 From: Thomas Vargiu Date: Tue, 14 Apr 2020 11:42:51 +0200 Subject: [PATCH 2/4] Use options parameter in PluginChain --- src/PluginChain.php | 4 ++-- src/PluginClient.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/PluginChain.php b/src/PluginChain.php index c6971aa..3decdaf 100644 --- a/src/PluginChain.php +++ b/src/PluginChain.php @@ -27,11 +27,11 @@ final class PluginChain * @param callable $clientCallable * @param int $maxRestarts */ - public function __construct(array $plugins, callable $clientCallable, int $maxRestarts) + public function __construct(array $plugins, callable $clientCallable, array $options = []) { $this->plugins = $plugins; $this->clientCallable = $clientCallable; - $this->maxRestarts = $maxRestarts; + $this->maxRestarts = (int) ($options['max_restarts'] ?? 0); } private function createChain(): callable diff --git a/src/PluginClient.php b/src/PluginClient.php index 67ff020..c325c3d 100644 --- a/src/PluginClient.php +++ b/src/PluginClient.php @@ -125,6 +125,6 @@ private function configure(array $options = []): array */ private function createPluginChain(array $pluginList, callable $clientCallable): callable { - return new PluginChain($pluginList, $clientCallable, $this->options['max_restarts']); + return new PluginChain($pluginList, $clientCallable, $this->options); } } From c72dd683a6a9771a17f9a14235031345bee7ef63 Mon Sep 17 00:00:00 2001 From: Thomas Vargiu Date: Tue, 14 Apr 2020 13:00:02 +0200 Subject: [PATCH 3/4] Fixed plugin invoke order with tests --- src/PluginChain.php | 4 +- tests/PluginChainTest.php | 93 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 tests/PluginChainTest.php diff --git a/src/PluginChain.php b/src/PluginChain.php index 3decdaf..207fd30 100644 --- a/src/PluginChain.php +++ b/src/PluginChain.php @@ -7,6 +7,7 @@ use Http\Client\Common\Exception\LoopException; use Http\Promise\Promise; use Psr\Http\Message\RequestInterface; +use function array_reverse; final class PluginChain { @@ -37,8 +38,9 @@ public function __construct(array $plugins, callable $clientCallable, array $opt private function createChain(): callable { $lastCallable = $this->clientCallable; + $reversedPlugins = array_reverse($this->plugins); - foreach ($this->plugins as $plugin) { + foreach ($reversedPlugins as $plugin) { $lastCallable = function (RequestInterface $request) use ($plugin, $lastCallable) { return $plugin->handleRequest($request, $lastCallable, $this); }; diff --git a/tests/PluginChainTest.php b/tests/PluginChainTest.php new file mode 100644 index 0000000..95fdfab --- /dev/null +++ b/tests/PluginChainTest.php @@ -0,0 +1,93 @@ +func = $func; + } + + public function handleRequest(RequestInterface $request, callable $next, callable $first): Promise + { + ($this->func)($request, $next, $first); + + return $next($request); + } + }; + } + + public function testChainShouldInvokePluginsInReversedOrder(): void + { + $pluginOrderCalls = []; + + $plugin1 = $this->createPlugin(static function () use (&$pluginOrderCalls) { + $pluginOrderCalls[] = 'plugin1'; + }); + $plugin2 = $this->createPlugin(static function () use (&$pluginOrderCalls) { + $pluginOrderCalls[] = 'plugin2'; + }); + + $request = $this->prophesize(RequestInterface::class); + $responsePromise = $this->prophesize(Promise::class); + + $clientCallable = static function () use ($responsePromise) { + return $responsePromise->reveal(); + }; + + $pluginOrderCalls = []; + + $plugins = [ + $plugin1, + $plugin2, + ]; + + $pluginChain = new PluginChain($plugins, $clientCallable); + + $result = $pluginChain($request->reveal()); + + $this->assertSame($responsePromise->reveal(), $result); + $this->assertSame(['plugin1', 'plugin2'], $pluginOrderCalls); + } + + public function testShouldThrowLoopExceptionOnMaxRestarts(): void + { + $this->expectException(LoopException::class); + + $request = $this->prophesize(RequestInterface::class); + $responsePromise = $this->prophesize(Promise::class); + $calls = 0; + $clientCallable = static function () use ($responsePromise, &$calls) { + ++$calls; + + return $responsePromise->reveal(); + }; + + $pluginChain = new PluginChain([], $clientCallable, ['max_restarts' => 2]); + + $pluginChain($request->reveal()); + $this->assertSame(1, $calls); + $pluginChain($request->reveal()); + $this->assertSame(2, $calls); + $pluginChain($request->reveal()); + $this->assertSame(3, $calls); + $pluginChain($request->reveal()); + } +} From fb6b543fc7cd856ba5943b9a20e504e55eab8260 Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Wed, 1 Jul 2020 15:19:04 +0100 Subject: [PATCH 4/4] Fixes --- src/PluginChain.php | 11 +++++++---- src/PluginClient.php | 13 ++++++------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/PluginChain.php b/src/PluginChain.php index 207fd30..220ba36 100644 --- a/src/PluginChain.php +++ b/src/PluginChain.php @@ -4,10 +4,10 @@ namespace Http\Client\Common; +use function array_reverse; use Http\Client\Common\Exception\LoopException; use Http\Promise\Promise; use Psr\Http\Message\RequestInterface; -use function array_reverse; final class PluginChain { @@ -24,9 +24,12 @@ final class PluginChain private $restarts = 0; /** - * @param Plugin[] $plugins - * @param callable $clientCallable - * @param int $maxRestarts + * @param Plugin[] $plugins A plugin chain + * @param callable $clientCallable Callable making the HTTP call + * @param array $options { + * + * @var int $max_restarts + * } */ public function __construct(array $plugins, callable $clientCallable, array $options = []) { diff --git a/src/PluginClient.php b/src/PluginClient.php index c325c3d..0d330b1 100644 --- a/src/PluginClient.php +++ b/src/PluginClient.php @@ -4,7 +4,6 @@ namespace Http\Client\Common; -use Http\Client\Common\Exception\LoopException; use Http\Client\Exception as HttplugException; use Http\Client\HttpAsyncClient; use Http\Client\HttpClient; @@ -44,11 +43,11 @@ final class PluginClient implements HttpClient, HttpAsyncClient private $options; /** - * @param ClientInterface|HttpAsyncClient $client - * @param Plugin[] $plugins + * @param ClientInterface|HttpAsyncClient $client An HTTP async client + * @param Plugin[] $plugins A plugin chain * @param array $options { * - * @var int $max_restarts + * @var int $max_restarts * } */ public function __construct($client, array $plugins = [], array $options = []) @@ -120,11 +119,11 @@ private function configure(array $options = []): array /** * Create the plugin chain. * - * @param Plugin[] $pluginList A list of plugins + * @param Plugin[] $plugins A plugin chain * @param callable $clientCallable Callable making the HTTP call */ - private function createPluginChain(array $pluginList, callable $clientCallable): callable + private function createPluginChain(array $plugins, callable $clientCallable): callable { - return new PluginChain($pluginList, $clientCallable, $this->options); + return new PluginChain($plugins, $clientCallable, $this->options); } }