Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle out-of-order destruction of Fibers/DebugScopes during final shutdown #1209

Merged
merged 3 commits into from
Jan 13, 2024

Conversation

Nevay
Copy link
Contributor

@Nevay Nevay commented Jan 11, 2024

Disable warnings for missing calls to ::detach() during the final shutdown phase if the scope is not destructed in the original fiber; DebugScope::__destruct() might be called before the fiber is resumed / before finally blocks that ::detach() the scope are run.
I wasn't able to create a simple reproducer; it happens for example with amphp/file ^3.0 and tbachert/otel-async-revolt-adapter 0.1.1.

<?php
require_once __DIR__ . '/vendor/autoload.php';
Amp\File\openFile(__FILE__, 'rb');
... shutdown functions finished
scope 116 ::__destruct() (created in fiber 112, currently in fiber {main})
scope 116 ::detach() (created in fiber 112, currently in fiber 112)
scope 242 ::detach() (created in fiber 148, currently in fiber 148)
scope 242 ::__destruct() (created in fiber 148, currently in fiber 148)
Notice: Scope: missing call to Scope::detach() for scope #116, created
        at OpenTelemetry.Context.Context.activate(Context.php:87)
        at Nevay.Otel.Async.Revolt.RevoltDriver.Nevay.Otel.Async.Revolt.{closure}(RevoltDriver.php:43)
        at Revolt.EventLoop.Internal.AbstractDriver.invokeMicrotasks(AbstractDriver.php:425)
        at Revolt.EventLoop.Internal.AbstractDriver.Revolt.EventLoop.Internal.{closure}(AbstractDriver.php:562)
        at Fiber.resume(Unknown Source)
        at Revolt.EventLoop.Internal.DriverSuspension.Revolt.EventLoop.Internal.{closure}(DriverSuspension.php:64)
        at Revolt.EventLoop.Internal.AbstractDriver.invokeMicrotasks(AbstractDriver.php:425)
        at Revolt.EventLoop.Internal.AbstractDriver.Revolt.EventLoop.Internal.{closure}(AbstractDriver.php:616)
        at Fiber.resume(Unknown Source)
        at Revolt.EventLoop.Internal.DriverSuspension.Revolt.EventLoop.Internal.{closure}(DriverSuspension.php:64)
        at Revolt.EventLoop.Internal.AbstractDriver.invokeMicrotasks(AbstractDriver.php:425)
        at Revolt.EventLoop.Internal.AbstractDriver.Revolt.EventLoop.Internal.{closure}(AbstractDriver.php:616)
        at Fiber.resume(Unknown Source)
        at Revolt.EventLoop.Internal.AbstractDriver.invokeCallbacks(AbstractDriver.php:497)
        at Revolt.EventLoop.Internal.AbstractDriver.Revolt.EventLoop.Internal.{closure}(AbstractDriver.php:553)
        at Fiber.resume(Unknown Source)
        at Revolt.EventLoop.Internal.AbstractDriver.Revolt.EventLoop.Internal.{closure}(AbstractDriver.php:94)
        at Revolt.EventLoop.Internal.DriverSuspension.suspend(DriverSuspension.php:117)
        at Amp.Future.await(Future.php:251)
        at Amp.File.Driver.ParallelFilesystemDriver.selectWorker(ParallelFilesystemDriver.php:79)
        at Amp.File.Driver.ParallelFilesystemDriver.openFile(ParallelFilesystemDriver.php:49)
        at Amp.File.Driver.StatusCachingFilesystemDriver.openFile(StatusCachingFilesystemDriver.php:22)
        at Amp.File.Filesystem.openFile(Filesystem.php:21)
        at Amp.File.openFile(functions.php:80)
        at {main}(test.php:3)

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (596d4a2) 83.29% compared to head (a04b3e9) 83.26%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1209      +/-   ##
============================================
- Coverage     83.29%   83.26%   -0.03%     
- Complexity     2250     2256       +6     
============================================
  Files           285      285              
  Lines          6404     6417      +13     
============================================
+ Hits           5334     5343       +9     
- Misses         1070     1074       +4     
Flag Coverage Δ
7.4 81.89% <65.00%> (-0.08%) ⬇️
8.0 83.16% <65.00%> (-0.08%) ⬇️
8.1 83.32% <75.00%> (-0.05%) ⬇️
8.2 83.32% <75.00%> (-0.05%) ⬇️
8.3 83.32% <75.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Context/DebugScope.php 91.80% <80.00%> (-6.12%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 596d4a2...a04b3e9. Read the comment docs.

@Nevay Nevay marked this pull request as ready for review January 12, 2024 15:02
@Nevay Nevay requested a review from a team January 12, 2024 15:02
@brettmc brettmc merged commit 925ae9b into open-telemetry:main Jan 13, 2024
11 of 13 checks passed
@Nevay Nevay deleted the feature/debug-scope-fiber-shutdown branch January 13, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants