From 22e2d2b176bb2f1db8b619d4c92e45ad400a713f Mon Sep 17 00:00:00 2001 From: prolic Date: Sun, 22 Oct 2017 20:12:07 +0800 Subject: [PATCH 1/3] fix bugs in event bus resolves https://github.com/prooph/service-bus/issues/169 --- src/EventBus.php | 36 +++++++++- src/Plugin/InvokeStrategy/OnEventStrategy.php | 21 ++++-- tests/EventBusTest.php | 2 +- ...mMessageEventHandlerThrowingExceptions.php | 21 ++++++ .../InvokeStrategy/OnEventStrategyTest.php | 70 +++++++++++++++++++ 5 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 tests/Mock/CustomMessageEventHandlerThrowingExceptions.php diff --git a/src/EventBus.php b/src/EventBus.php index 884c1dd..9549e51 100644 --- a/src/EventBus.php +++ b/src/EventBus.php @@ -30,6 +30,11 @@ class EventBus extends MessageBus */ protected $collectExceptions = false; + /** + * @var array + */ + protected $collectedExceptions = []; + public function __construct(ActionEventEmitter $actionEventEmitter = null) { parent::__construct($actionEventEmitter); @@ -58,12 +63,29 @@ function (ActionEvent $actionEvent): void { $actionEvent->setParam(self::EVENT_PARAM_MESSAGE_HANDLED, true); } - if (count($caughtExceptions)) { - throw EventListenerException::collected(...$caughtExceptions); + foreach ($caughtExceptions as $ex) { + $this->collectedExceptions[] = $ex; } }, self::PRIORITY_INVOKE_HANDLER ); + + $this->events->attachListener( + self::EVENT_FINALIZE, + function (ActionEvent $actionEvent): void { + $target = $actionEvent->getTarget(); + + if (empty($target->collectedExceptions)) { + return; + } + + $exceptions = $target->collectedExceptions; + $target->collectedExceptions = []; + + $actionEvent->setParam(MessageBus::EVENT_PARAM_EXCEPTION, EventListenerException::collected(...$exceptions)); + }, + 1000 + ); } /** @@ -99,4 +121,14 @@ public function disableCollectExceptions(): void { $this->collectExceptions = false; } + + public function isCollectingException(): bool + { + return $this->collectExceptions; + } + + public function addCollectedException(\Throwable $e): void + { + $this->collectedExceptions[] = $e; + } } diff --git a/src/Plugin/InvokeStrategy/OnEventStrategy.php b/src/Plugin/InvokeStrategy/OnEventStrategy.php index b160fdf..14ecce1 100644 --- a/src/Plugin/InvokeStrategy/OnEventStrategy.php +++ b/src/Plugin/InvokeStrategy/OnEventStrategy.php @@ -24,15 +24,24 @@ public function attachToMessageBus(MessageBus $messageBus): void $this->listenerHandlers[] = $messageBus->attach( MessageBus::EVENT_DISPATCH, function (ActionEvent $actionEvent): void { - if ($actionEvent->getParam(MessageBus::EVENT_PARAM_MESSAGE_HANDLED, false)) { - return; - } - + $target = $actionEvent->getTarget(); $message = $actionEvent->getParam(MessageBus::EVENT_PARAM_MESSAGE); $handlers = $actionEvent->getParam(EventBus::EVENT_PARAM_EVENT_LISTENERS, []); - foreach ($handlers as $handler) { - $handler->onEvent($message); + try { + foreach ($handlers as $handler) { + if (is_callable($handler)) { + continue; + } + + $handler->onEvent($message); + } + } catch (\Throwable $e) { + if ($target->isCollectingException()) { + $target->addCollectedException($e); + } else { + throw $e; + } } $actionEvent->setParam(MessageBus::EVENT_PARAM_MESSAGE_HANDLED, true); diff --git a/tests/EventBusTest.php b/tests/EventBusTest.php index 9492b5f..2478fca 100644 --- a/tests/EventBusTest.php +++ b/tests/EventBusTest.php @@ -273,7 +273,7 @@ public function it_collects_exceptions_if_mode_is_enabled(): void MessageBus::EVENT_DISPATCH, function (ActionEvent $e) use ($handler, $errorProducer): void { if ($e->getParam(MessageBus::EVENT_PARAM_MESSAGE_NAME) === CustomMessage::class) { - $e->setParam(EventBus::EVENT_PARAM_EVENT_LISTENERS, [$handler, $errorProducer, $handler]); + $e->setParam(EventBus::EVENT_PARAM_EVENT_LISTENERS, [$handler, $errorProducer, $handler]); } }, MessageBus::PRIORITY_ROUTE diff --git a/tests/Mock/CustomMessageEventHandlerThrowingExceptions.php b/tests/Mock/CustomMessageEventHandlerThrowingExceptions.php new file mode 100644 index 0000000..e74f761 --- /dev/null +++ b/tests/Mock/CustomMessageEventHandlerThrowingExceptions.php @@ -0,0 +1,21 @@ + + * (c) 2015-2017 Sascha-Oliver Prolic + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ProophTest\ServiceBus\Mock; + +final class CustomMessageEventHandlerThrowingExceptions +{ + public function onEvent($message): void + { + throw new \Exception('bar'); + } +} diff --git a/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php b/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php index 84f8064..8acf1e7 100644 --- a/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php +++ b/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php @@ -15,11 +15,15 @@ use PHPUnit\Framework\TestCase; use Prooph\Common\Event\DefaultListenerHandler; use Prooph\ServiceBus\EventBus; +use Prooph\ServiceBus\Exception\EventListenerException; +use Prooph\ServiceBus\Exception\MessageDispatchException; use Prooph\ServiceBus\Plugin\InvokeStrategy\OnEventStrategy; +use Prooph\ServiceBus\Plugin\ListenerExceptionCollectionMode; use Prooph\ServiceBus\Plugin\Router\EventRouter; use ProophTest\ServiceBus\Mock\CustomInvokableMessageHandler; use ProophTest\ServiceBus\Mock\CustomMessage; use ProophTest\ServiceBus\Mock\CustomMessageEventHandler; +use ProophTest\ServiceBus\Mock\CustomMessageEventHandlerThrowingExceptions; use Prophecy\Argument; class OnEventStrategyTest extends TestCase @@ -91,4 +95,70 @@ public function it_should_not_handle_already_processed_messages(): void $this->assertSame($customEvent, $callableHandler->getLastMessage()); $this->assertSame(1, $callableHandler->getInvokeCounter()); } + + /** + * @test + */ + public function it_should_still_work_with_callables(): void + { + $eventBus = new EventBus(); + + $onEventStrategy = new OnEventStrategy(); + $onEventStrategy->attachToMessageBus($eventBus); + + $handler = new CustomMessageEventHandler(); + + $result = false; + + $router = new EventRouter(); + $router->route(CustomMessage::class) + ->to(function (CustomMessage $message) use (&$result) { + $result = true; + }) + ->andTo($handler); + + $router->attachToMessageBus($eventBus); + + $eventBus->dispatch(new CustomMessage('some text')); + + $this->assertTrue($result); + $this->assertSame(1, $handler->getInvokeCounter()); + } + + /** + * @test + */ + public function it_should_still_work_with_callables_and_collect_all_exceptions(): void + { + $eventBus = new EventBus(); + + $exceptionModePlugin = new ListenerExceptionCollectionMode(); + $exceptionModePlugin->attachToMessageBus($eventBus); + + $onEventStrategy = new OnEventStrategy(); + $onEventStrategy->attachToMessageBus($eventBus); + + $handler = new CustomMessageEventHandlerThrowingExceptions(); + + $router = new EventRouter(); + $router->route(CustomMessage::class) + ->to(function (CustomMessage $message) { + throw new \Exception('foo'); + }) + ->andTo($handler); + + $router->attachToMessageBus($eventBus); + + $ex = null; + + try { + $eventBus->dispatch(new CustomMessage('some text')); + } catch (MessageDispatchException $ex) { + $ex = $ex->getPrevious(); + } + + $this->assertNotNull($ex); + $this->assertInstanceOf(EventListenerException::class, $ex); + $this->assertCount(2, $ex->listenerExceptions()); + } } From 6b3da958517a47161a782f32f281f926d3ae9a3f Mon Sep 17 00:00:00 2001 From: prolic Date: Mon, 23 Oct 2017 00:23:39 +0800 Subject: [PATCH 2/3] live-bugfixing part 2 --- src/Plugin/InvokeStrategy/OnEventStrategy.php | 22 +++--- tests/Mock/CustomMessageEventHandler2.php | 36 +++++++++ tests/Mock/CustomOnEventStrategy.php | 52 +++++++++++++ .../InvokeStrategy/OnEventStrategyTest.php | 75 +++++++++++++++++++ 4 files changed, 174 insertions(+), 11 deletions(-) create mode 100644 tests/Mock/CustomMessageEventHandler2.php create mode 100644 tests/Mock/CustomOnEventStrategy.php diff --git a/src/Plugin/InvokeStrategy/OnEventStrategy.php b/src/Plugin/InvokeStrategy/OnEventStrategy.php index 14ecce1..19f7df0 100644 --- a/src/Plugin/InvokeStrategy/OnEventStrategy.php +++ b/src/Plugin/InvokeStrategy/OnEventStrategy.php @@ -28,19 +28,19 @@ function (ActionEvent $actionEvent): void { $message = $actionEvent->getParam(MessageBus::EVENT_PARAM_MESSAGE); $handlers = $actionEvent->getParam(EventBus::EVENT_PARAM_EVENT_LISTENERS, []); - try { - foreach ($handlers as $handler) { - if (is_callable($handler)) { - continue; - } + foreach ($handlers as $handler) { + if (is_callable($handler) || ! is_object($handler) || ! method_exists($handler, 'onEvent')) { + continue; + } + try { $handler->onEvent($message); - } - } catch (\Throwable $e) { - if ($target->isCollectingException()) { - $target->addCollectedException($e); - } else { - throw $e; + } catch (\Throwable $e) { + if ($target->isCollectingException()) { + $target->addCollectedException($e); + } else { + throw $e; + } } } diff --git a/tests/Mock/CustomMessageEventHandler2.php b/tests/Mock/CustomMessageEventHandler2.php new file mode 100644 index 0000000..76b5238 --- /dev/null +++ b/tests/Mock/CustomMessageEventHandler2.php @@ -0,0 +1,36 @@ + + * (c) 2015-2017 Sascha-Oliver Prolic + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ProophTest\ServiceBus\Mock; + +final class CustomMessageEventHandler2 +{ + private $lastMessage; + + private $invokeCounter = 0; + + public function on($message): void + { + $this->lastMessage = $message; + $this->invokeCounter++; + } + + public function getLastMessage() + { + return $this->lastMessage; + } + + public function getInvokeCounter(): int + { + return $this->invokeCounter; + } +} diff --git a/tests/Mock/CustomOnEventStrategy.php b/tests/Mock/CustomOnEventStrategy.php new file mode 100644 index 0000000..75249b6 --- /dev/null +++ b/tests/Mock/CustomOnEventStrategy.php @@ -0,0 +1,52 @@ + + * (c) 2015-2017 Sascha-Oliver Prolic + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ProophTest\ServiceBus\Mock; + +use Prooph\Common\Event\ActionEvent; +use Prooph\ServiceBus\EventBus; +use Prooph\ServiceBus\MessageBus; +use Prooph\ServiceBus\Plugin\AbstractPlugin; + +final class CustomOnEventStrategy extends AbstractPlugin +{ + public function attachToMessageBus(MessageBus $messageBus): void + { + $this->listenerHandlers[] = $messageBus->attach( + MessageBus::EVENT_DISPATCH, + function (ActionEvent $actionEvent): void { + $target = $actionEvent->getTarget(); + $message = $actionEvent->getParam(MessageBus::EVENT_PARAM_MESSAGE); + $handlers = $actionEvent->getParam(EventBus::EVENT_PARAM_EVENT_LISTENERS, []); + + foreach ($handlers as $handler) { + if (is_callable($handler) || ! is_object($handler) || ! method_exists($handler, 'on')) { + continue; + } + + try { + $handler->on($message); + } catch (\Throwable $e) { + if ($target->isCollectingException()) { + $target->addCollectedException($e); + } else { + throw $e; + } + } + } + + $actionEvent->setParam(MessageBus::EVENT_PARAM_MESSAGE_HANDLED, true); + }, + MessageBus::PRIORITY_INVOKE_HANDLER + ); + } +} diff --git a/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php b/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php index 8acf1e7..964068e 100644 --- a/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php +++ b/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php @@ -23,7 +23,9 @@ use ProophTest\ServiceBus\Mock\CustomInvokableMessageHandler; use ProophTest\ServiceBus\Mock\CustomMessage; use ProophTest\ServiceBus\Mock\CustomMessageEventHandler; +use ProophTest\ServiceBus\Mock\CustomMessageEventHandler2; use ProophTest\ServiceBus\Mock\CustomMessageEventHandlerThrowingExceptions; +use ProophTest\ServiceBus\Mock\CustomOnEventStrategy; use Prophecy\Argument; class OnEventStrategyTest extends TestCase @@ -161,4 +163,77 @@ public function it_should_still_work_with_callables_and_collect_all_exceptions() $this->assertInstanceOf(EventListenerException::class, $ex); $this->assertCount(2, $ex->listenerExceptions()); } + + /** + * @test + */ + public function it_should_still_work_with_callables_and_collect_all_exceptions_part2(): void + { + $eventBus = new EventBus(); + + $exceptionModePlugin = new ListenerExceptionCollectionMode(); + $exceptionModePlugin->attachToMessageBus($eventBus); + + $onEventStrategy = new OnEventStrategy(); + $onEventStrategy->attachToMessageBus($eventBus); + + $handler = new CustomMessageEventHandlerThrowingExceptions(); + + $router = new EventRouter(); + $router->route(CustomMessage::class) + ->to(function (CustomMessage $message) { + throw new \Exception('foo'); + }) + ->andTo($handler) + ->andTo($handler); + + $router->attachToMessageBus($eventBus); + + $ex = null; + + try { + $eventBus->dispatch(new CustomMessage('some text')); + } catch (MessageDispatchException $ex) { + $ex = $ex->getPrevious(); + } + + $this->assertNotNull($ex); + $this->assertInstanceOf(EventListenerException::class, $ex); + $this->assertCount(3, $ex->listenerExceptions()); + } + + /** + * @test + */ + public function it_should_still_work_with_callables_and_other_strategies(): void + { + $eventBus = new EventBus(); + + $onEventStrategy = new OnEventStrategy(); + $onEventStrategy->attachToMessageBus($eventBus); + + $secondOnEventStrategy = new CustomOnEventStrategy(); + $secondOnEventStrategy->attachToMessageBus($eventBus); + + $handler = new CustomMessageEventHandler(); + $handler2 = new CustomMessageEventHandler2(); + + $result = false; + + $router = new EventRouter(); + $router->route(CustomMessage::class) + ->to(function (CustomMessage $message) use (&$result) { + $result = true; + }) + ->andTo($handler) + ->andTo($handler2); + + $router->attachToMessageBus($eventBus); + + $eventBus->dispatch(new CustomMessage('some text')); + + $this->assertTrue($result); + $this->assertSame(1, $handler->getInvokeCounter()); + $this->assertSame(1, $handler2->getInvokeCounter()); + } } From 1ed3321042c18843c741c935eb4c3fa2d8307d5d Mon Sep 17 00:00:00 2001 From: prolic Date: Mon, 23 Oct 2017 16:38:56 +0800 Subject: [PATCH 3/3] rename method to isCollectingExceptions, add return void to test functions --- src/EventBus.php | 2 +- src/Plugin/InvokeStrategy/OnEventStrategy.php | 4 ++-- tests/Mock/CustomOnEventStrategy.php | 4 ++-- tests/Plugin/InvokeStrategy/OnEventStrategyTest.php | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/EventBus.php b/src/EventBus.php index 9549e51..ca0637c 100644 --- a/src/EventBus.php +++ b/src/EventBus.php @@ -122,7 +122,7 @@ public function disableCollectExceptions(): void $this->collectExceptions = false; } - public function isCollectingException(): bool + public function isCollectingExceptions(): bool { return $this->collectExceptions; } diff --git a/src/Plugin/InvokeStrategy/OnEventStrategy.php b/src/Plugin/InvokeStrategy/OnEventStrategy.php index 19f7df0..0f88294 100644 --- a/src/Plugin/InvokeStrategy/OnEventStrategy.php +++ b/src/Plugin/InvokeStrategy/OnEventStrategy.php @@ -29,14 +29,14 @@ function (ActionEvent $actionEvent): void { $handlers = $actionEvent->getParam(EventBus::EVENT_PARAM_EVENT_LISTENERS, []); foreach ($handlers as $handler) { - if (is_callable($handler) || ! is_object($handler) || ! method_exists($handler, 'onEvent')) { + if (is_callable($handler) || ! is_object($handler) || ! is_callable([$handler, 'onEvent'])) { continue; } try { $handler->onEvent($message); } catch (\Throwable $e) { - if ($target->isCollectingException()) { + if ($target->isCollectingExceptions()) { $target->addCollectedException($e); } else { throw $e; diff --git a/tests/Mock/CustomOnEventStrategy.php b/tests/Mock/CustomOnEventStrategy.php index 75249b6..6449adf 100644 --- a/tests/Mock/CustomOnEventStrategy.php +++ b/tests/Mock/CustomOnEventStrategy.php @@ -29,14 +29,14 @@ function (ActionEvent $actionEvent): void { $handlers = $actionEvent->getParam(EventBus::EVENT_PARAM_EVENT_LISTENERS, []); foreach ($handlers as $handler) { - if (is_callable($handler) || ! is_object($handler) || ! method_exists($handler, 'on')) { + if (is_callable($handler) || ! is_object($handler) || ! is_callable([$handler, 'on'])) { continue; } try { $handler->on($message); } catch (\Throwable $e) { - if ($target->isCollectingException()) { + if ($target->isCollectingExceptions()) { $target->addCollectedException($e); } else { throw $e; diff --git a/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php b/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php index 964068e..b4e3c57 100644 --- a/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php +++ b/tests/Plugin/InvokeStrategy/OnEventStrategyTest.php @@ -66,7 +66,7 @@ public function it_can_be_attached_to_event_bus(): void ->shouldBeCalled() ->willReturn( new DefaultListenerHandler( - function () { + function (): void { } ) ); @@ -114,7 +114,7 @@ public function it_should_still_work_with_callables(): void $router = new EventRouter(); $router->route(CustomMessage::class) - ->to(function (CustomMessage $message) use (&$result) { + ->to(function (CustomMessage $message) use (&$result): void { $result = true; }) ->andTo($handler); @@ -144,7 +144,7 @@ public function it_should_still_work_with_callables_and_collect_all_exceptions() $router = new EventRouter(); $router->route(CustomMessage::class) - ->to(function (CustomMessage $message) { + ->to(function (CustomMessage $message): void { throw new \Exception('foo'); }) ->andTo($handler); @@ -181,7 +181,7 @@ public function it_should_still_work_with_callables_and_collect_all_exceptions_p $router = new EventRouter(); $router->route(CustomMessage::class) - ->to(function (CustomMessage $message) { + ->to(function (CustomMessage $message): void { throw new \Exception('foo'); }) ->andTo($handler) @@ -222,7 +222,7 @@ public function it_should_still_work_with_callables_and_other_strategies(): void $router = new EventRouter(); $router->route(CustomMessage::class) - ->to(function (CustomMessage $message) use (&$result) { + ->to(function (CustomMessage $message) use (&$result): void { $result = true; }) ->andTo($handler)