Skip to content

Commit adf651e

Browse files
committed
Simplify common case of immediately entering the span
This PR allows the following API: ``` let _guard = tracing::span!("foo").entered(); ``` See tokio-rs#1246 for an extended discussion.
1 parent 1b800fa commit adf651e

File tree

2 files changed

+155
-31
lines changed

2 files changed

+155
-31
lines changed

tracing/src/span.rs

Lines changed: 117 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,8 @@ use core::{
322322
cmp, fmt,
323323
hash::{Hash, Hasher},
324324
marker::PhantomData,
325+
mem,
326+
ops::Deref,
325327
};
326328

327329
/// Trait implemented by types which have a span `Id`.
@@ -379,7 +381,36 @@ pub(crate) struct Inner {
379381
#[must_use = "once a span has been entered, it should be exited"]
380382
pub struct Entered<'a> {
381383
span: &'a Span,
382-
_not_send: PhantomData<*mut ()>,
384+
385+
/// ```compile_fail
386+
/// use tracing::span::*;
387+
/// trait AssertSend: Send {}
388+
///
389+
/// impl AssertSend for Entered<'_> {}
390+
/// ```
391+
_not_send: PhantomNotSend,
392+
}
393+
394+
/// An owned version of [`Entered`], a guard representing a span which has been
395+
/// entered and is currently executing.
396+
///
397+
/// When the guard is dropped, the span will be exited.
398+
///
399+
/// This is returned by the [`Span::entered`] function.
400+
///
401+
/// [`Span::entered`]: super::Span::entered()
402+
#[derive(Debug)]
403+
#[must_use = "once a span has been entered, it should be exited"]
404+
pub struct EnteredSpan {
405+
span: Span,
406+
407+
/// ```compile_fail
408+
/// use tracing::span::*;
409+
/// trait AssertSend: Send {}
410+
///
411+
/// impl AssertSend for EnteredSpan {}
412+
/// ```
413+
_not_send: PhantomNotSend,
383414
}
384415

385416
/// `log` target for all span lifecycle (creation/enter/exit/close) records.
@@ -758,7 +789,25 @@ impl Span {
758789
/// [`Collect::exit`]: super::collect::Collect::exit()
759790
/// [`Id`]: super::Id
760791
pub fn enter(&self) -> Entered<'_> {
761-
if let Some(ref inner) = self.inner.as_ref() {
792+
self.do_enter();
793+
Entered {
794+
span: self,
795+
_not_send: PhantomNotSend,
796+
}
797+
}
798+
799+
/// An owned version of [`Entered`].
800+
pub fn entered(self) -> EnteredSpan {
801+
self.do_enter();
802+
EnteredSpan {
803+
span: self,
804+
_not_send: PhantomNotSend,
805+
}
806+
}
807+
808+
#[inline]
809+
fn do_enter(&self) {
810+
if let Some(inner) = self.inner.as_ref() {
762811
inner.collector.enter(&inner.id);
763812
}
764813

@@ -767,11 +816,23 @@ impl Span {
767816
self.log(ACTIVITY_LOG_TARGET, log::Level::Trace, format_args!("-> {}", meta.name()));
768817
}
769818
}}
819+
}
770820

771-
Entered {
772-
span: self,
773-
_not_send: PhantomData,
821+
// Called from [`Entered`] and [`EnteredSpan`] drops.
822+
//
823+
// Running this behaviour on drop rather than with an explicit function
824+
// call means that spans may still be exited when unwinding.
825+
#[inline]
826+
fn do_exit(&self) {
827+
if let Some(inner) = self.inner.as_ref() {
828+
inner.collector.exit(&inner.id);
774829
}
830+
831+
if_log_enabled! { crate::Level::TRACE, {
832+
if let Some(ref _meta) = self.meta {
833+
self.log(ACTIVITY_LOG_TARGET, log::Level::Trace, format_args!("<- {}", _meta.name()));
834+
}
835+
}}
775836
}
776837

777838
/// Executes the given function in the context of this span.
@@ -1223,42 +1284,65 @@ impl Clone for Inner {
12231284

12241285
// ===== impl Entered =====
12251286

1226-
/// # Safety
1227-
///
1228-
/// Technically, `Entered` _can_ implement both `Send` *and* `Sync` safely. It
1229-
/// doesn't, because it has a `PhantomData<*mut ()>` field, specifically added
1230-
/// in order to make it `!Send`.
1287+
impl EnteredSpan {
1288+
/// Exits this span, returning the underlying [`Span`].
1289+
#[inline]
1290+
pub fn exit(mut self) -> Span {
1291+
// One does not simply move out of a struct with `Drop`.
1292+
let span = mem::replace(&mut self.span, Span::none());
1293+
span.do_exit();
1294+
span
1295+
}
1296+
}
1297+
1298+
impl Deref for EnteredSpan {
1299+
type Target = Span;
1300+
1301+
#[inline]
1302+
fn deref(&self) -> &Span {
1303+
&self.span
1304+
}
1305+
}
1306+
1307+
impl<'a> Drop for Entered<'a> {
1308+
#[inline]
1309+
fn drop(&mut self) {
1310+
self.span.do_exit()
1311+
}
1312+
}
1313+
1314+
impl Drop for EnteredSpan {
1315+
#[inline]
1316+
fn drop(&mut self) {
1317+
self.span.do_exit()
1318+
}
1319+
}
1320+
1321+
/// Technically, `Entered` (or `EnteredSpan`) _can_ implement both `Send` *and*
1322+
/// `Sync` safely. It doesn't, because it has a `PhantomNotSend` field,
1323+
/// specifically added in order to make it `!Send`.
12311324
///
12321325
/// Sending an `Entered` guard between threads cannot cause memory unsafety.
12331326
/// However, it *would* result in incorrect behavior, so we add a
1234-
/// `PhantomData<*mut ()>` to prevent it from being sent between threads. This
1235-
/// is because it must be *dropped* on the same thread that it was created;
1327+
/// `PhantomNotSend` to prevent it from being sent between threads. This is
1328+
/// because it must be *dropped* on the same thread that it was created;
12361329
/// otherwise, the span will never be exited on the thread where it was entered,
12371330
/// and it will attempt to exit the span on a thread that may never have entered
12381331
/// it. However, we still want them to be `Sync` so that a struct holding an
12391332
/// `Entered` guard can be `Sync`.
12401333
///
12411334
/// Thus, this is totally safe.
1242-
unsafe impl<'a> Sync for Entered<'a> {}
1243-
1244-
impl<'a> Drop for Entered<'a> {
1245-
#[inline]
1246-
fn drop(&mut self) {
1247-
// Dropping the guard exits the span.
1248-
//
1249-
// Running this behaviour on drop rather than with an explicit function
1250-
// call means that spans may still be exited when unwinding.
1251-
if let Some(inner) = self.span.inner.as_ref() {
1252-
inner.collector.exit(&inner.id);
1253-
}
1254-
1255-
if let Some(ref _meta) = self.span.meta {
1256-
if_log_enabled! { crate::Level::TRACE, {
1257-
self.span.log(ACTIVITY_LOG_TARGET, log::Level::Trace, format_args!("<- {}", _meta.name()));
1258-
}}
1259-
}
1260-
}
1335+
#[derive(Debug)]
1336+
struct PhantomNotSend {
1337+
ghost: PhantomData<*mut ()>,
12611338
}
1339+
#[allow(non_upper_case_globals)]
1340+
const PhantomNotSend: PhantomNotSend = PhantomNotSend { ghost: PhantomData };
1341+
1342+
/// # Safety
1343+
///
1344+
/// Trivially safe, as `PhantomNotSend` doesn't have any API.
1345+
unsafe impl Sync for PhantomNotSend {}
12621346

12631347
#[cfg(feature = "log")]
12641348
struct FmtValues<'a>(&'a Record<'a>);
@@ -1301,4 +1385,6 @@ mod test {
13011385

13021386
trait AssertSync: Sync {}
13031387
impl AssertSync for Span {}
1388+
impl AssertSync for Entered<'_> {}
1389+
impl AssertSync for EnteredSpan {}
13041390
}

tracing/tests/span.rs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,44 @@ fn enter() {
297297
handle.assert_finished();
298298
}
299299

300+
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
301+
#[test]
302+
fn entered() {
303+
let (collector, handle) = collector::mock()
304+
.enter(span::mock().named("foo"))
305+
.event(event::mock())
306+
.exit(span::mock().named("foo"))
307+
.drop_span(span::mock().named("foo"))
308+
.done()
309+
.run_with_handle();
310+
with_default(collector, || {
311+
let _span = span!(Level::TRACE, "foo").entered();
312+
debug!("dropping guard...");
313+
});
314+
315+
handle.assert_finished();
316+
}
317+
318+
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
319+
#[test]
320+
fn entered_api() {
321+
let (collector, handle) = collector::mock()
322+
.enter(span::mock().named("foo"))
323+
.event(event::mock())
324+
.exit(span::mock().named("foo"))
325+
.drop_span(span::mock().named("foo"))
326+
.done()
327+
.run_with_handle();
328+
with_default(collector, || {
329+
let span = span!(Level::TRACE, "foo").entered();
330+
let _derefs_to_span = span.id();
331+
debug!("exiting span...");
332+
let _: Span = span.exit();
333+
});
334+
335+
handle.assert_finished();
336+
}
337+
300338
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
301339
#[test]
302340
fn moved_field() {

0 commit comments

Comments
 (0)