From 6cd7a8af2cbe03374053f5de96104216a9001cd8 Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Tue, 9 Apr 2024 01:11:29 +0200 Subject: [PATCH] Improve fiber bound context storage (#1270) * Improve fiber bound context storage Now works properly in fibers once initial context is attached. * Change `Context::storage()` return type to `ContextStorageInterface` `ExecutionContextAwareInterface` should not be relevant for end-users / it was mainly exposed for the FFI fiber handler; calling any of its method with enabled fiber handler would have broken the storage. Swoole context storage README creates a new storage instead of wrapping `Context::storage()`: `Context::setStorage(new SwooleContextStorage(new ContextStorage()));`. * Add BC layer for execution context aware fiber storage * Fix BC layer inactive execution context detection for {main} --- src/Context/Context.php | 2 +- src/Context/ContextStorage.php | 9 ++- src/Context/ContextStorageHead.php | 2 +- src/Context/ContextStorageHeadAware.php | 13 ++++ src/Context/ContextStorageNode.php | 14 ++-- src/Context/FiberBoundContextStorage.php | 72 +++++++++--------- ...berBoundContextStorageExecutionAwareBC.php | 74 +++++++++++++++++++ src/Context/FiberBoundContextStorageScope.php | 64 ---------------- src/Context/README.md | 4 +- src/Context/ZendObserverFiber.php | 11 ++- .../fiber/initialize_fiber_handler.php | 15 +--- .../test_context_switching_not_supported.phpt | 16 +++- ..._not_trigger_if_attached_before_usage.phpt | 67 +++++++++++++++++ tests/Unit/Context/DebugScopeTest.php | 14 +--- ...oundContextStorageExecutionAwareBCTest.php | 68 +++++++++++++++++ tests/Unit/Context/ScopeTest.php | 9 +-- 16 files changed, 304 insertions(+), 150 deletions(-) create mode 100644 src/Context/ContextStorageHeadAware.php create mode 100644 src/Context/FiberBoundContextStorageExecutionAwareBC.php delete mode 100644 src/Context/FiberBoundContextStorageScope.php create mode 100644 tests/Integration/Context/Fiber/test_context_switching_not_supported_does_not_trigger_if_attached_before_usage.phpt create mode 100644 tests/Unit/Context/FiberBoundContextStorageExecutionAwareBCTest.php diff --git a/src/Context/Context.php b/src/Context/Context.php index 55715f03..e64e533a 100644 --- a/src/Context/Context.php +++ b/src/Context/Context.php @@ -44,7 +44,7 @@ final class Context implements ContextInterface public static function storage(): ContextStorageInterface&ExecutionContextAwareInterface { /** @psalm-suppress RedundantPropertyInitializationCheck */ - return self::$storage ??= new ContextStorage(); + return self::$storage ??= new FiberBoundContextStorageExecutionAwareBC(); } /** diff --git a/src/Context/ContextStorage.php b/src/Context/ContextStorage.php index 1f905789..614cd37a 100644 --- a/src/Context/ContextStorage.php +++ b/src/Context/ContextStorage.php @@ -7,9 +7,9 @@ namespace OpenTelemetry\Context; /** * @internal */ -final class ContextStorage implements ContextStorageInterface, ExecutionContextAwareInterface +final class ContextStorage implements ContextStorageInterface, ContextStorageHeadAware, ExecutionContextAwareInterface { - public ContextStorageHead $current; + private ContextStorageHead $current; private ContextStorageHead $main; /** @var array */ private array $forks = []; @@ -34,6 +34,11 @@ final class ContextStorage implements ContextStorageInterface, ExecutionContextA unset($this->forks[$id]); } + public function head(): ContextStorageHead + { + return $this->current; + } + public function scope(): ?ContextStorageScopeInterface { return ($this->current->node->head ?? null) === $this->current diff --git a/src/Context/ContextStorageHead.php b/src/Context/ContextStorageHead.php index a942496d..729f72ff 100644 --- a/src/Context/ContextStorageHead.php +++ b/src/Context/ContextStorageHead.php @@ -11,7 +11,7 @@ final class ContextStorageHead { public ?ContextStorageNode $node = null; - public function __construct(public ContextStorage $storage) + public function __construct(public ContextStorageHeadAware $storage) { } } diff --git a/src/Context/ContextStorageHeadAware.php b/src/Context/ContextStorageHeadAware.php new file mode 100644 index 00000000..32f6532d --- /dev/null +++ b/src/Context/ContextStorageHeadAware.php @@ -0,0 +1,13 @@ +localStorage[$offset]); } - /** - * @phan-suppress PhanUndeclaredClassAttribute - */ - #[\ReturnTypeWillChange] - public function offsetGet($offset) + public function offsetGet(mixed $offset): mixed { return $this->localStorage[$offset]; } - public function offsetSet($offset, $value): void + public function offsetSet(mixed $offset, mixed $value): void { $this->localStorage[$offset] = $value; } - public function offsetUnset($offset): void + public function offsetUnset(mixed $offset): void { unset($this->localStorage[$offset]); } @@ -52,7 +48,7 @@ final class ContextStorageNode implements ScopeInterface, ContextStorageScopeInt public function detach(): int { $flags = 0; - if ($this->head !== $this->head->storage->current) { + if ($this->head !== $this->head->storage->head()) { $flags |= ScopeInterface::INACTIVE; } diff --git a/src/Context/FiberBoundContextStorage.php b/src/Context/FiberBoundContextStorage.php index d55aa039..483cb8bd 100644 --- a/src/Context/FiberBoundContextStorage.php +++ b/src/Context/FiberBoundContextStorage.php @@ -1,77 +1,79 @@ */ + private WeakMap $heads; + + public function __construct() { + $this->heads = new WeakMap(); + $this->heads[$this] = new ContextStorageHead($this); } - public function fork(int|string $id): void + public function head(): ?ContextStorageHead { - $this->storage->fork($id); - } - - public function switch(int|string $id): void - { - $this->storage->switch($id); - } - - public function destroy(int|string $id): void - { - $this->storage->destroy($id); + return $this->heads[Fiber::getCurrent() ?? $this] ?? null; } public function scope(): ?ContextStorageScopeInterface { - $this->checkFiberMismatch(); + $head = $this->heads[Fiber::getCurrent() ?? $this] ?? null; + + if (!$head?->node && Fiber::getCurrent()) { + self::triggerNotInitializedFiberContextWarning(); - if (($scope = $this->storage->scope()) === null) { return null; } - return new FiberBoundContextStorageScope($scope); + // Starts with empty head instead of cloned parent -> no need to check for head mismatch + return $head->node; } public function current(): ContextInterface { - $this->checkFiberMismatch(); + $head = $this->heads[Fiber::getCurrent() ?? $this] ?? null; - return $this->storage->current(); + if (!$head?->node && Fiber::getCurrent()) { + self::triggerNotInitializedFiberContextWarning(); + + // Fallback to {main} to preserve BC + $head = $this->heads[$this]; + } + + return $head->node->context ?? Context::getRoot(); } public function attach(ContextInterface $context): ContextStorageScopeInterface { - $scope = $this->storage->attach($context); - assert(class_exists(Fiber::class, false)); - $scope[Fiber::class] = Fiber::getCurrent(); + $head = $this->heads[Fiber::getCurrent() ?? $this] ??= new ContextStorageHead($this); - return new FiberBoundContextStorageScope($scope); + return $head->node = new ContextStorageNode($context, $head, $head->node); } - private function checkFiberMismatch(): void + private static function triggerNotInitializedFiberContextWarning(): void { - $scope = $this->storage->scope(); - assert(class_exists(Fiber::class, false)); - if ($scope && $scope[Fiber::class] !== Fiber::getCurrent()) { - trigger_error('Fiber context switching not supported', E_USER_WARNING); - } + $fiber = Fiber::getCurrent(); + assert($fiber !== null); + + trigger_error(sprintf( + 'Access to not initialized OpenTelemetry context in fiber (id: %d), automatic forking not supported, must attach initial fiber context manually', + spl_object_id($fiber), + ), E_USER_WARNING); } } diff --git a/src/Context/FiberBoundContextStorageExecutionAwareBC.php b/src/Context/FiberBoundContextStorageExecutionAwareBC.php new file mode 100644 index 00000000..fe54b226 --- /dev/null +++ b/src/Context/FiberBoundContextStorageExecutionAwareBC.php @@ -0,0 +1,74 @@ +storage = new FiberBoundContextStorage(); + } + + public function fork(int|string $id): void + { + $this->bcStorage()->fork($id); + } + + public function switch(int|string $id): void + { + $this->bcStorage()->switch($id); + } + + public function destroy(int|string $id): void + { + $this->bcStorage()->destroy($id); + } + + private function bcStorage(): ContextStorage + { + if ($this->bc === null) { + $this->bc = new ContextStorage(); + + // Copy head into $this->bc storage to preserve already attached scopes + /** @psalm-suppress PossiblyNullFunctionCall */ + $head = (static fn ($storage) => $storage->heads[$storage]) + ->bindTo(null, FiberBoundContextStorage::class)($this->storage); + $head->storage = $this->bc; + + /** @psalm-suppress PossiblyNullFunctionCall */ + (static fn ($storage) => $storage->current = $storage->main = $head) + ->bindTo(null, ContextStorage::class)($this->bc); + } + + return $this->bc; + } + + public function scope(): ?ContextStorageScopeInterface + { + return $this->bc + ? $this->bc->scope() + : $this->storage->scope(); + } + + public function current(): ContextInterface + { + return $this->bc + ? $this->bc->current() + : $this->storage->current(); + } + + public function attach(ContextInterface $context): ContextStorageScopeInterface + { + return $this->bc + ? $this->bc->attach($context) + : $this->storage->attach($context); + } +} diff --git a/src/Context/FiberBoundContextStorageScope.php b/src/Context/FiberBoundContextStorageScope.php deleted file mode 100644 index f4708f21..00000000 --- a/src/Context/FiberBoundContextStorageScope.php +++ /dev/null @@ -1,64 +0,0 @@ -scope->offsetExists($offset); - } - - /** - * @phan-suppress PhanUndeclaredClassAttribute - */ - #[\ReturnTypeWillChange] - public function offsetGet($offset) - { - return $this->scope->offsetGet($offset); - } - - public function offsetSet($offset, $value): void - { - $this->scope->offsetSet($offset, $value); - } - - public function offsetUnset($offset): void - { - $this->scope->offsetUnset($offset); - } - - public function context(): ContextInterface - { - return $this->scope->context(); - } - - public function detach(): int - { - $flags = $this->scope->detach(); - assert(class_exists(Fiber::class, false)); - if ($this->scope[Fiber::class] !== Fiber::getCurrent()) { - $flags |= ScopeInterface::INACTIVE; - } - - return $flags; - } -} diff --git a/src/Context/README.md b/src/Context/README.md index 8a1ea258..d589dc0a 100644 --- a/src/Context/README.md +++ b/src/Context/README.md @@ -39,9 +39,9 @@ truthy value. Disabling is only recommended for applications using `exit` / `die ## Async applications -### Fiber support +### Fiber support - automatic context propagation to newly created fibers -Requires `PHP >= 8.1`, an NTS build, `ext-ffi`, and setting the environment variable `OTEL_PHP_FIBERS_ENABLED` to a truthy value. Additionally `vendor/autoload.php` has to be preloaded for non-CLI SAPIs if [`ffi.enable`](https://www.php.net/manual/en/ffi.configuration.php#ini.ffi.enable) is set to `preload`. +Requires an NTS build, `ext-ffi`, and setting the environment variable `OTEL_PHP_FIBERS_ENABLED` to a truthy value. Additionally `vendor/autoload.php` has to be preloaded for non-CLI SAPIs if [`ffi.enable`](https://www.php.net/manual/en/ffi.configuration.php#ini.ffi.enable) is set to `preload`. ### Event loops diff --git a/src/Context/ZendObserverFiber.php b/src/Context/ZendObserverFiber.php index 4d3d0c5e..b700307e 100644 --- a/src/Context/ZendObserverFiber.php +++ b/src/Context/ZendObserverFiber.php @@ -48,7 +48,7 @@ final class ZendObserverFiber try { $fibers = FFI::scope('OTEL_ZEND_OBSERVER_FIBER'); - } catch (FFI\Exception $e) { + } catch (FFI\Exception) { try { $fibers = FFI::load(__DIR__ . '/fiber/zend_observer_fiber.h'); } catch (FFI\Exception $e) { @@ -58,9 +58,12 @@ final class ZendObserverFiber } } - $fibers->zend_observer_fiber_init_register(static fn (int $initializing) => Context::storage()->fork($initializing)); //@phpstan-ignore-line - $fibers->zend_observer_fiber_switch_register(static fn (int $from, int $to) => Context::storage()->switch($to)); //@phpstan-ignore-line - $fibers->zend_observer_fiber_destroy_register(static fn (int $destroying) => Context::storage()->destroy($destroying)); //@phpstan-ignore-line + $storage = new ContextStorage(); + $fibers->zend_observer_fiber_init_register(static fn (int $initializing) => $storage->fork($initializing)); //@phpstan-ignore-line + $fibers->zend_observer_fiber_switch_register(static fn (int $from, int $to) => $storage->switch($to)); //@phpstan-ignore-line + $fibers->zend_observer_fiber_destroy_register(static fn (int $destroying) => $storage->destroy($destroying)); //@phpstan-ignore-line + + Context::setStorage($storage); return true; } diff --git a/src/Context/fiber/initialize_fiber_handler.php b/src/Context/fiber/initialize_fiber_handler.php index b9c70639..e7768227 100644 --- a/src/Context/fiber/initialize_fiber_handler.php +++ b/src/Context/fiber/initialize_fiber_handler.php @@ -1,20 +1,9 @@ activate(); $fiber = new Fiber(function() use ($key) { + echo 'fiber(pre):' . Context::getCurrent()->get($key), PHP_EOL; + $scope = Context::getCurrent() ->with($key, 'fiber') ->activate(); @@ -26,6 +28,8 @@ $fiber = new Fiber(function() use ($key) { echo 'fiber:' . Context::getCurrent()->get($key), PHP_EOL; $scope->detach(); + + echo 'fiber(post):' . Context::getCurrent()->get($key), PHP_EOL; }); echo 'main:' . Context::getCurrent()->get($key), PHP_EOL; @@ -42,10 +46,14 @@ $scope->detach(); --EXPECTF-- main:main -Warning: Fiber context switching not supported in %s -fiber:fiber +Warning: Access to not initialized OpenTelemetry context in fiber (id: %d), automatic forking not supported, must attach initial fiber context manually %s +fiber(pre):main -Warning: Fiber context switching not supported in %s -main:fiber +Warning: Access to not initialized OpenTelemetry context in fiber (id: %d), automatic forking not supported, must attach initial fiber context manually %s fiber:fiber main:main +fiber:fiber + +Warning: Access to not initialized OpenTelemetry context in fiber (id: %d), automatic forking not supported, must attach initial fiber context manually %s +fiber(post):main +main:main diff --git a/tests/Integration/Context/Fiber/test_context_switching_not_supported_does_not_trigger_if_attached_before_usage.phpt b/tests/Integration/Context/Fiber/test_context_switching_not_supported_does_not_trigger_if_attached_before_usage.phpt new file mode 100644 index 00000000..bfe0a749 --- /dev/null +++ b/tests/Integration/Context/Fiber/test_context_switching_not_supported_does_not_trigger_if_attached_before_usage.phpt @@ -0,0 +1,67 @@ +--TEST-- +Context usage in fiber without fiber support does not trigger warning if context is attached before usage. +--SKIPIF-- + +--ENV-- +OTEL_PHP_FIBERS_ENABLED=0 +--FILE-- +with($key, 'main') + ->activate(); + +$fiber = new Fiber(bindContext(function() use ($key) { + echo 'fiber(pre):' . Context::getCurrent()->get($key), PHP_EOL; + + $scope = Context::getCurrent() + ->with($key, 'fiber') + ->activate(); + + echo 'fiber:' . Context::getCurrent()->get($key), PHP_EOL; + + Fiber::suspend(); + echo 'fiber:' . Context::getCurrent()->get($key), PHP_EOL; + + $scope->detach(); + + echo 'fiber(post):' . Context::getCurrent()->get($key), PHP_EOL; +})); + +echo 'main:' . Context::getCurrent()->get($key), PHP_EOL; + +$fiber->start(); +echo 'main:' . Context::getCurrent()->get($key), PHP_EOL; + +$fiber->resume(); +echo 'main:' . Context::getCurrent()->get($key), PHP_EOL; + +$scope->detach(); + + +// see https://github.com/opentelemetry-php/context?tab=readme-ov-file#event-loops +function bindContext(Closure $closure): Closure { + $context = Context::getCurrent(); + return static function (mixed ...$args) use ($closure, $context): mixed { + $scope = $context->activate(); + try { + return $closure(...$args); + } finally { + $scope->detach(); + } + }; +} + +?> +--EXPECTF-- +main:main +fiber(pre):main +fiber:fiber +main:main +fiber:fiber +fiber(post):main +main:main diff --git a/tests/Unit/Context/DebugScopeTest.php b/tests/Unit/Context/DebugScopeTest.php index 90082a74..35d1a06f 100644 --- a/tests/Unit/Context/DebugScopeTest.php +++ b/tests/Unit/Context/DebugScopeTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace OpenTelemetry\Tests\Unit\Context; use Exception; +use Fiber; use function ini_set; use OpenTelemetry\Context\Context; use OpenTelemetry\Context\DebugScope; @@ -104,17 +105,10 @@ final class DebugScopeTest extends TestCase { $scope1 = Context::getCurrent()->activate(); - Context::storage()->fork(1); - Context::storage()->switch(1); + $this->expectException(Exception::class); + $this->expectExceptionMessage('different execution context'); - try { - $this->expectException(Exception::class); - $this->expectExceptionMessage('different execution context'); - $scope1->detach(); - } finally { - Context::storage()->switch(0); - Context::storage()->destroy(1); - } + (new Fiber($scope1->detach(...)))->start(); } public function test_missing_scope_detach(): void diff --git a/tests/Unit/Context/FiberBoundContextStorageExecutionAwareBCTest.php b/tests/Unit/Context/FiberBoundContextStorageExecutionAwareBCTest.php new file mode 100644 index 00000000..844b08d0 --- /dev/null +++ b/tests/Unit/Context/FiberBoundContextStorageExecutionAwareBCTest.php @@ -0,0 +1,68 @@ +attach($storage->current()); + + $scope = $storage->scope(); + + $storage->fork(1); + + $this->assertSame($scope, $storage->scope()); + } + + public function test_inactive_scope_detach(): void + { + $storage = new FiberBoundContextStorageExecutionAwareBC(); + $scope1 = $storage->attach($storage->current()); + + $storage->fork(1); + $storage->switch(1); + $this->assertSame(ScopeInterface::INACTIVE, @$scope1->detach() & ScopeInterface::INACTIVE); + } + + public function test_storage_switch_switches_context(): void + { + $storage = new FiberBoundContextStorageExecutionAwareBC(); + $main = $storage->current(); + $fork = $storage->current()->with(Context::createKey('-'), 42); + + $scopeMain = $storage->attach($main); + + // Coroutine start + $storage->fork(1); + $storage->switch(1); + $this->assertSame($main, $storage->current()); + + $scopeFork = $storage->attach($fork); + $this->assertSame($fork, $storage->current()); + + // Coroutine suspend + $storage->switch(0); + $this->assertSame($main, $storage->current()); + + // Coroutine resume + $storage->switch(1); + $this->assertSame($fork, $storage->current()); + + $scopeFork->detach(); + + // Coroutine return + $storage->switch(0); + $storage->destroy(1); + + $scopeMain->detach(); + } +} diff --git a/tests/Unit/Context/ScopeTest.php b/tests/Unit/Context/ScopeTest.php index 08149ada..17a0d76c 100644 --- a/tests/Unit/Context/ScopeTest.php +++ b/tests/Unit/Context/ScopeTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace OpenTelemetry\Tests\Unit\Context; +use Fiber; use OpenTelemetry\Context\Context; use OpenTelemetry\Context\ContextStorage; use OpenTelemetry\Context\ScopeInterface; @@ -83,12 +84,10 @@ class ScopeTest extends TestCase { $scope1 = Context::getCurrent()->activate(); - Context::storage()->fork(1); - Context::storage()->switch(1); - $this->assertSame(ScopeInterface::INACTIVE, @$scope1->detach() & ScopeInterface::INACTIVE); + $fiber = new Fiber(static fn () => @$scope1->detach()); + $fiber->start(); - Context::storage()->switch(0); - Context::storage()->destroy(1); + $this->assertSame(ScopeInterface::INACTIVE, $fiber->getReturn() & ScopeInterface::INACTIVE); } public function test_scope_context_returns_context_of_scope(): void