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

tracing, tracing-futures: instrument futures with Spans in Drop #2561

Closed
wants to merge 4 commits into from
Closed

tracing, tracing-futures: instrument futures with Spans in Drop #2561

wants to merge 4 commits into from

Conversation

zohnannor
Copy link

Resolves #2541

Motivation

See #2541

Solution

Wrap Instrumented::inner (both in tracing and tracing-futures) into Option. Pin::set it to None in the PinnedDrop impl to drop the future in the context of a Span that we enter in the said impl.

This implementation uses Option::unwrap_unchecked in places where we need to extract inner future, as the only time we set it to None is in Drop, or in a method which takes Instrumented by value, guaranteeing that there is no case for UB.

Alternative was to use ManuallyDrop, but I don't think we can Future::poll Pin<&mut ManuallyDrop<T>> easily?

@zohnannor
Copy link
Author

Closed in favor of #2562

@zohnannor zohnannor closed this Apr 14, 2023
@zohnannor zohnannor deleted the instrumented-drop branch April 18, 2023 00:23
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.

Consider entering Span on Drop for Instrumented
1 participant