From 9c0d7e865d792269f0302a7442559cd643fdef10 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Tue, 19 Jul 2022 10:08:23 +1000 Subject: [PATCH 1/3] store root dispatcher in a static var This seems to be the safest way to store the main dispatcher. Using context to store the main event dispatcher causes a scope to be created: activating the newly-created context which stores dispatcher causes a new scope, and that can break things. I also looked at storing dispatcher in the root context, but that can't work because contexts are immutable. --- src/API/Common/Event/Dispatcher.php | 23 +++------------- src/API/Common/Event/EmitsEventsTrait.php | 2 +- tests/Benchmark/EventBench.php | 2 +- .../Common/EventContextIntegrationTest.php | 26 +++++++++++++++++++ .../Unit/API/Common/Event/DispatcherTest.php | 8 +++--- .../API/Common/Event/EmitsEventsTraitTest.php | 2 +- 6 files changed, 36 insertions(+), 27 deletions(-) create mode 100644 tests/Integration/API/Common/EventContextIntegrationTest.php diff --git a/src/API/Common/Event/Dispatcher.php b/src/API/Common/Event/Dispatcher.php index 6d45888a2..b34c1a706 100644 --- a/src/API/Common/Event/Dispatcher.php +++ b/src/API/Common/Event/Dispatcher.php @@ -5,20 +5,16 @@ namespace OpenTelemetry\API\Common\Event; use CloudEvents\V1\CloudEventInterface; -use OpenTelemetry\Context\Context; -use OpenTelemetry\Context\ContextKey; class Dispatcher implements DispatcherInterface { - private static ?ContextKey $key = null; + private static ?self $root = null; /** @var array>> */ private array $listeners = []; - public static function getInstance(): self + public static function getRoot(): self { - $key = self::getConstantKeyInstance(); - - return Context::getCurrent()->get($key) ?? self::createInstance(); + return self::$root ??= new self(); } public function dispatch(CloudEventInterface $event): void @@ -47,17 +43,4 @@ private function dispatchEvent(iterable $listeners, CloudEventInterface $event): $listener($event); } } - - private static function getConstantKeyInstance(): ContextKey - { - return self::$key ??= new ContextKey(self::class); - } - - private static function createInstance(): self - { - $dispatcher = new self(); - Context::getCurrent()->with(self::getConstantKeyInstance(), $dispatcher)->activate(); - - return $dispatcher; - } } diff --git a/src/API/Common/Event/EmitsEventsTrait.php b/src/API/Common/Event/EmitsEventsTrait.php index f55a23266..e55c6f205 100644 --- a/src/API/Common/Event/EmitsEventsTrait.php +++ b/src/API/Common/Event/EmitsEventsTrait.php @@ -10,6 +10,6 @@ trait EmitsEventsTrait { protected static function emit(CloudEventInterface $event): void { - Dispatcher::getInstance()->dispatch($event); + Dispatcher::getRoot()->dispatch($event); } } diff --git a/tests/Benchmark/EventBench.php b/tests/Benchmark/EventBench.php index cb4e46568..28f8c2a99 100644 --- a/tests/Benchmark/EventBench.php +++ b/tests/Benchmark/EventBench.php @@ -17,7 +17,7 @@ class EventBench public function __construct() { - $this->dispatcher = Dispatcher::getInstance(); + $this->dispatcher = Dispatcher::getRoot(); $this->listener = function () { }; $this->event = new CloudEvent(uniqid(), self::class, 'foo'); diff --git a/tests/Integration/API/Common/EventContextIntegrationTest.php b/tests/Integration/API/Common/EventContextIntegrationTest.php new file mode 100644 index 000000000..ab7575179 --- /dev/null +++ b/tests/Integration/API/Common/EventContextIntegrationTest.php @@ -0,0 +1,26 @@ +with($key, 1)->activate(); + Dispatcher::getRoot(); + $result = $scope->detach(); + + $this->assertSame(0, $result); + $this->assertNull(Context::getCurrent()->get($key)); + } +} diff --git a/tests/Unit/API/Common/Event/DispatcherTest.php b/tests/Unit/API/Common/Event/DispatcherTest.php index 20708d9e4..d10e92ef9 100644 --- a/tests/Unit/API/Common/Event/DispatcherTest.php +++ b/tests/Unit/API/Common/Event/DispatcherTest.php @@ -33,18 +33,18 @@ public function setUp(): void public function test_get_instance(): void { - $dispatcher = Dispatcher::getInstance(); + $dispatcher = Dispatcher::getRoot(); $this->assertInstanceOf(Dispatcher::class, $dispatcher); - $this->assertSame($dispatcher, Dispatcher::getInstance()); + $this->assertSame($dispatcher, Dispatcher::getRoot()); } public function test_get_instance_from_parent_context(): void { - $dispatcher = Dispatcher::getInstance(); + $dispatcher = Dispatcher::getRoot(); $this->assertInstanceOf(Dispatcher::class, $dispatcher); $parent = Context::getCurrent()->with(new ContextKey('foo'), 'bar'); $parent->activate(); - $this->assertSame($dispatcher, Dispatcher::getInstance()); + $this->assertSame($dispatcher, Dispatcher::getRoot()); } public function test_add_listener(): void diff --git a/tests/Unit/API/Common/Event/EmitsEventsTraitTest.php b/tests/Unit/API/Common/Event/EmitsEventsTraitTest.php index 9d06dca58..fee48d2ac 100644 --- a/tests/Unit/API/Common/Event/EmitsEventsTraitTest.php +++ b/tests/Unit/API/Common/Event/EmitsEventsTraitTest.php @@ -20,7 +20,7 @@ public function test_emits_event(): void $event->method('getType')->willReturn('bar'); $called = false; $class = $this->createInstance(); - Dispatcher::getInstance()->listen($event->getType(), function () use (&$called) { + Dispatcher::getRoot()->listen($event->getType(), function () use (&$called) { $this->assertTrue(true, 'listener was called'); $called = true; }); From 67d40e626e390fa990479e01fb57e80720992d73 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Tue, 19 Jul 2022 10:21:04 +1000 Subject: [PATCH 2/3] remove useless test --- tests/Unit/API/Common/Event/DispatcherTest.php | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/tests/Unit/API/Common/Event/DispatcherTest.php b/tests/Unit/API/Common/Event/DispatcherTest.php index d10e92ef9..e5a4472e9 100644 --- a/tests/Unit/API/Common/Event/DispatcherTest.php +++ b/tests/Unit/API/Common/Event/DispatcherTest.php @@ -31,22 +31,13 @@ public function setUp(): void $this->event->method('getType')->willReturn('foo'); } - public function test_get_instance(): void + public function test_get_root_dispatcher(): void { $dispatcher = Dispatcher::getRoot(); $this->assertInstanceOf(Dispatcher::class, $dispatcher); $this->assertSame($dispatcher, Dispatcher::getRoot()); } - public function test_get_instance_from_parent_context(): void - { - $dispatcher = Dispatcher::getRoot(); - $this->assertInstanceOf(Dispatcher::class, $dispatcher); - $parent = Context::getCurrent()->with(new ContextKey('foo'), 'bar'); - $parent->activate(); - $this->assertSame($dispatcher, Dispatcher::getRoot()); - } - public function test_add_listener(): void { $listenerFunction = function () { From 8f76d2a2220751305f4820434d044f346f76be56 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Tue, 19 Jul 2022 10:27:20 +1000 Subject: [PATCH 3/3] linting --- tests/Unit/API/Common/Event/DispatcherTest.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/Unit/API/Common/Event/DispatcherTest.php b/tests/Unit/API/Common/Event/DispatcherTest.php index e5a4472e9..d7e45a30f 100644 --- a/tests/Unit/API/Common/Event/DispatcherTest.php +++ b/tests/Unit/API/Common/Event/DispatcherTest.php @@ -6,8 +6,6 @@ use CloudEvents\V1\CloudEventInterface; use OpenTelemetry\API\Common\Event\Dispatcher; -use OpenTelemetry\Context\Context; -use OpenTelemetry\Context\ContextKey; use PHPUnit\Framework\TestCase; /**