subscriber: don't abort in release when cloning a closed span#3567
subscriber: don't abort in release when cloning a closed span#3567ameyypawar wants to merge 3 commits into
Conversation
…rs#3514) `Registry::clone_span` uses `assert_ne!` to check that a span's reference count is non-zero before incrementing it. Holding a span open across an `.await` point can leave a stale entry on the span stack after the span's reference count has reached zero; a later `Span::current()` then clones that closed span and the `assert_ne!` aborts the worker thread, even in release builds. This is incorrect API usage, but it should not crash the program. Downgrade the assertion to `debug_assert_ne!`, so debug builds still surface the misuse during development while release builds tolerate the (incorrect but non-fatal) resurrected span instead of aborting. Adds a regression test covering both the debug and release behavior.
…-rs#3514) Reword the comment on the downgraded assertion to describe it as a caller bug that must not abort the program, rather than claiming the resurrected span is harmless. Also note in the regression test that only the release branch distinguishes this fix from the previous `assert_ne!` (both panic under debug), so it must be run with `cargo test --release` to exercise the non-aborting path.
| fn clone_span_with_zero_ref_count_does_not_abort_in_release() { | ||
| let dispatch = dispatcher::Dispatch::new(Registry::default()); | ||
| dispatcher::with_default(&dispatch, || { | ||
| let span = tracing::info_span!("held across await"); |
There was a problem hiding this comment.
this is a fairly minor nit, but the test does not actually hold the span across an await point, so i might not name it that. the test reproduces the behavior that holding a span across an await might cause, but does so by cloning the span ID manually after dropping it. i think this is fine, for the record: adding an async block here is unnecessary and would just make the actual behavior less obvious. but i'd prefer to rename this.
| if cfg!(debug_assertions) { | ||
| // Debug builds keep the assertion to surface the misuse. | ||
| assert!( | ||
| cloned.is_err(), | ||
| "cloning a closed span should hit the debug assertion in debug builds", | ||
| ); | ||
| } else { | ||
| // Release builds must not panic; the clone returns the same ID. | ||
| assert_eq!( | ||
| cloned.expect("cloning a closed span must not panic in release builds"), | ||
| id, | ||
| ); | ||
| } |
There was a problem hiding this comment.
hmm, i don't love having a test that makes completely different assertions across debug and release builds, since that means that in order to test all the expected behaviors, one must build and run the tests in both debug and release mode. my gut reaction is to say that we should remove the test, but i also don't love that, it does seem fair to want to test that the behavior doesn't cause a panic. that makes me begin to wonder whether it's even worth keeping the assertion in debug mode. it does seem valuable to help catch bugs in user code while running tests, but... hmm. maybe we were just being too aggressive by panicking in this case and should always just ignore it? what do you think?
…o-rs#3514) Following review feedback, drop the assertion entirely rather than panicking in debug builds. Holding a span across an `.await` point is incorrect usage but easy to do by accident, and it should not abort the program in any build mode. `clone_span` now tolerates a zero reference count, keeping the `fetch_add` (which stays balanced with the matching `try_close`). This also lets the regression test make the same assertion in every build mode (removing the debug-vs-release split), and renames the test span so it no longer implies an actual await point.
|
Thanks for the review! Renamed the test span — you're right that On the debug/release split: I agree it isn't worth keeping the assertion. Holding a span across an One thing worth flagging: the assertion also guarded against internal reference-count bugs in the registry itself — a genuine double-close would now go unnoticed here rather than tripping in tests. I think that's an acceptable trade for not aborting on user misuse (and |
Motivation
Registry::clone_spanasserts viaassert_ne!that a span's reference count isnon-zero before incrementing it. Holding a span open across an
.awaitpoint —incorrect, but easy to do by accident — can leave a stale entry on the span
stack after the span's reference count has reached zero. A later
Span::current()then calls
clone_spanon the now-closed span, and theassert_ne!aborts theworker thread, even in release builds:
This started when #3289 removed the
clone_span/try_closebalancing from theRegistry'senter/exit— that change didn't account for spans enteredacross
.awaitpoints. Entering a span across an await point is incorrect usage,but it should not crash the program.
Fixes: #3514
Solution
Downgrade the assertion to
debug_assert_ne!, as suggested in the issue: debugbuilds still surface the misuse during development, while release builds keep
running instead of aborting. Keeping the
fetch_addleaves the reference countbalanced, so a later
try_closestill removes the span normally.A regression test reproduces the zero-reference-count clone against a bare
Registryand checks the debug-vs-release behavior. Note that only the releasebranch distinguishes this fix from the previous
assert_ne!(both panic underdebug), so it must be run with
cargo test --releaseto exercise thenon-aborting path.