Open ilslv opened 1 year ago
Here's a contrived example of the same problem happening with #[tracing::instrument]
showcasing two possible outcomes.
use std::future::{pending, ready};
use futures::FutureExt as _;
#[tracing::instrument]
async fn foo(switch: bool) -> i32 {
let val = LogOnDrop;
match switch {
true => ready(1).await,
false => pending::<i32>().await,
}
}
#[tokio::test]
async fn main() {
tracing_subscriber::fmt().init();
// polled to completion
assert!(foo(true).now_or_never().is_some());
// polled once and thrown away mid-execution
assert!(foo(false).now_or_never().is_none());
}
struct LogOnDrop;
impl Drop for LogOnDrop {
fn drop(&mut self) {
tracing::info!("drop");
}
}
Output:
2023-03-30T12:00:22.905817Z INFO foo{switch=true}: soc_common::future::tests: drop
2023-03-30T12:00:22.905874Z INFO soc_common::future::tests: drop
Hmm, this isn't a bad idea. I agree that it's potentially desirable to have any events occurring inside a Drop
implementation for an Instrument
ed future occur inside that span.
One potential challenge is that it would be necessary to actually ensure that the inner Future
's drop implementation runs while the span is entered. This means we would need to actually drop the inner Future
inside the drop
method on Instrumented
, while the guard is held, rather than after drop
returns. We could solve this either by putting the Future
inside an Option
, which is take
n in drop
so that it can be dropped inside drop
(haha), or using std::mem::ManuallyDrop
. Using ManuallyDrop
would require unsafe code, but using an Option
would mean that we would need to unwrap
the Option
every time the future is polled, adding some small per-poll overhead. Using ManuallyDrop
is probably a better solution here, since the behavior is simple enough and the use of unsafe code would be very low risk, as we can avoid the overhead of checking the Option
on every poll.
We could possibly avoid Option
check overhead by using .unwrap_or_else(|| unsafe { std::hint::unreachable_unchecked() })
, or, alternatively, just .unwrap_or_else(|| unreachable!())
, hinting at optimization to the compiler. ManuallyDrop
solution would be just as good (in terms of performance), but I don't see the reason to it given the simpler option (pun intended).
IMO using ManuallyDrop
is probably simpler and more idiomatic than unreachable_unchecked
— ManuallyDrop
is intended for situations where it's necessary to manually control drop order, which is what we need to do here. But either solution would be fine.
Opened PR #2561. I propose to discuss the implementation details of the feature there.
I actually myself already implemented version with ManuallyDrop
to avoid runtime and memory overhead https://github.com/tokio-rs/tracing/pull/2562
@ilslv as being released in 0.1.38, I guess we may close this issue now.
@tyranron I would like to leave this open as a reminder to do a similar thing to the WithDispatch
.
UPD: postponing the WithDispatch
changes because it will be a breaking change: https://github.com/tokio-rs/tracing/issues/2578
Feature Request
Crates
tracing
,tracing-futures
Motivation
Sometimes it's desired to get
Span
insideDrop
implementation. For example:The problem is there is difference between when
Drop
can be actually called: insideFuture::poll()
or onDrop
of the entireFuture
:Output:
Span is missing for a second log.
Proposal
Would you accept a PR adding
Drop
implementation forInstrumented
, that will enter theSpan
andDrop
innerFuture
?Alternatives
Leave it as is.