Skip to content

Conversation

@taisho6339
Copy link

@taisho6339 taisho6339 commented Nov 24, 2025

Fixes #2871

Problem

Context::current() panicked with "RefCell already mutably borrowed" when called from SpanProcessor::on_end during span cleanup. Root cause: ContextGuard::drop held a mutable borrow while dropping the context, which triggered Span::drop → on_end → Context::current() → panic.

Solution

Modified ContextGuard::drop to extract the context outside the borrow_mut() scope before dropping it:

impl Drop for ContextGuard {
    fn drop(&mut self) {
        let _to_drop = CURRENT_CONTEXT.with(|stack| {
            stack.borrow_mut().pop_id(id)  // Extract only
        }); // borrow_mut released here
        // Drop happens here - safe to call Context::current()
    }
}

Changes

  • opentelemetry: Modified ContextGuard::drop and ContextStack::pop_id, added documentation
  • opentelemetry-sdk: Added SpanProcessor::on_end documentation with best practices

Note

  • Context::current() called during drop returns the parent context (documented behavior).
  • Best practice: Extract context info in on_start and store as span attributes.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@taisho6339 taisho6339 changed the title return parent context in on_end instead of causing panic fix: return parent context in on_end instead of causing panic Nov 24, 2025
@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.8%. Comparing base (df412fe) to head (6c6a35b).

Files with missing lines Patch % Lines
opentelemetry-sdk/src/trace/mod.rs 85.7% 1 Missing ⚠️
opentelemetry/src/context.rs 90.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3262     +/-   ##
=======================================
- Coverage   80.8%   80.8%   -0.1%     
=======================================
  Files        129     129             
  Lines      23203   23212      +9     
=======================================
+ Hits       18750   18756      +6     
- Misses      4453    4456      +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@taisho6339 taisho6339 marked this pull request as ready for review November 24, 2025 07:29
@taisho6339 taisho6339 requested a review from a team as a code owner November 24, 2025 07:29
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @taisho6339 and thank you for the PR. I have a few comments about the doc and solution.

I will close and reopen the PR to try to start our CI benchmarks.

@bantonsson
Copy link
Contributor

Closing and reopening to start benchmarks.

@bantonsson bantonsson closed this Nov 24, 2025
@bantonsson bantonsson reopened this Nov 24, 2025
@bantonsson bantonsson changed the title fix: return parent context in on_end instead of causing panic fix: return current context in on_end instead of causing panic Nov 24, 2025
@bantonsson
Copy link
Contributor

bantonsson commented Nov 25, 2025

Thank you @taisho6339. The Benchmark numbers look way better now (actually resulting in some benchmarks running faster).

I don't see exactly these changes on my MBP M4 Max, but a degradation between 2 and 8 percent which is totally acceptable.

name baseDuration changesDuration difference
context/has_active_span/in-cx/alt 8.4±0.06ns 8.4±0.02ns 0.0
context/has_active_span/in-cx/spec 4.2±0.17ns 4.4±0.16ns +4.0
context/has_active_span/no-cx/alt 8.5±0.36ns 8.4±0.07ns -0.99
context/has_active_span/no-cx/spec 4.4±0.19ns 4.7±0.24ns +7.0
context/has_active_span/no-sdk/alt 8.4±0.06ns 8.4±0.04ns 0.0
context/has_active_span/no-sdk/spec 4.4±0.20ns 4.7±0.22ns +7.0
context/is_recording/in-cx/alt 5.0±0.81ns 4.7±0.17ns -6.5
context/is_recording/in-cx/spec 6.6±0.28ns 6.9±0.51ns +5.0
context/is_recording/no-cx/alt 4.7±0.20ns 4.7±0.18ns -0.99
context/is_recording/no-cx/spec 6.6±0.29ns 7.2±0.18ns +9.0
context/is_recording/no-sdk/alt 4.7±0.13ns 4.7±0.12ns 0.0
context/is_recording/no-sdk/spec 6.6±0.21ns 7.2±0.56ns +10
context/is_sampled/in-cx/alt 8.7±0.03ns 8.7±0.03ns 0.0
context/is_sampled/in-cx/spec 4.4±0.12ns 4.7±0.22ns +6.0
context/is_sampled/no-cx/alt 8.7±0.03ns 8.7±0.03ns 0.0
context/is_sampled/no-cx/spec 4.7±0.16ns 5.0±0.26ns +7.0
context/is_sampled/no-sdk/alt 8.7±0.10ns 8.7±0.03ns 0.0
context/is_sampled/no-sdk/spec 4.7±0.18ns 5.0±0.25ns +7.0
context_attach/nested_cx/empty_cx 46.8±1.12ns 37.0±1.13ns -21
context_attach/nested_cx/single_value_cx 48.2±1.16ns 43.0±0.92ns -11
context_attach/nested_cx/span_cx 48.2±1.30ns 43.6±0.96ns -9.1
context_attach/out_of_order_cx_drop/empty_cx 41.4±0.93ns 36.9±0.82ns -11
context_attach/out_of_order_cx_drop/single_value_cx 42.4±1.78ns 41.2±1.12ns -2.9
context_attach/out_of_order_cx_drop/span_cx 42.7±1.50ns 41.4±1.08ns -2.9
context_attach/single_cx/empty_cx 24.2±0.44ns 19.5±0.87ns -19
context_attach/single_cx/single_value_cx 23.6±0.81ns 21.8±0.60ns -7.4
context_attach/single_cx/span_cx 23.6±0.93ns 22.4±0.36ns -5.7
telemetry_suppression/enter_telemetry_suppressed_scope 26.5±0.35ns 23.2±0.35ns -12
telemetry_suppression/is_current_telemetry_suppressed_false 0.9±0.04ns 0.8±0.02ns -0.99
telemetry_suppression/is_current_telemetry_suppressed_true 0.9±0.05ns 0.8±0.02ns -2.0
telemetry_suppression/normal_attach 29.9±0.83ns 26.8±1.06ns -9.9

Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from the comment in the tests.

@bantonsson
Copy link
Contributor

This needs a second review and merge @open-telemetry/rust-maintainers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trace SDK and Context access issue

2 participants