Skip to content

use replace instead of insert to add extension#14

Open
rasviitanen wants to merge 1 commit intoinanna-malick:masterfrom
ConfiguraAB:path-bug
Open

use replace instead of insert to add extension#14
rasviitanen wants to merge 1 commit intoinanna-malick:masterfrom
ConfiguraAB:path-bug

Conversation

@rasviitanen
Copy link

@rasviitanen rasviitanen commented Jan 13, 2021

Previously ExtensionsMut::insert was used without a guarantee that we had exclusive access to a SpanRef's extensions.
This could lead to a panic in the insert function in the case where the extension was inserted concurrently by multiple calls to eval_ctx.

By using replace the assertion is ignored and it no longer matters whether we have exclusive access or not to a span in eval_ctx.

Example

Suppose we have have two parallel calls A and B to eval_ctx:

A is called with a span-tree with the following spans: iter: [SpanId(1), SpanId(2), SpanId(3)]
B is called with a span-tree with the following spans: iter: [SpanId(1), SpanId(2), SpanId(4)]

Both A and B enters the for loop and we suppose that they both adds Span(1) and Span(2) to their path.

Then A continues to iterate over its spans and reaches Span(3), which matches with the arm that eventually executes:

// path: [SpanId(1), SpanId(2)]
for span_ref in path.into_iter() {
    let mut write_guard = span_ref.extensions_mut();
    write_guard.insert::<LazyTraceCtx<SpanId, TraceId>>(LazyTraceCtx(
        TraceCtx {
            trace_id: local_trace_root.trace_id.clone(),
            parent_span: None,
        },
    ));
}

Everything is OK so far.

Then B reaches the same loop once it proceeds to SpanId(4).
B then tries insert the extension for SpanId(1), but it is already inserted by A.
This is forbidden and insert will panic.

Solution

As LazyTraceCtx is private, there is no way for another Layer implementation to conflict with our extension, this means that we should be able to ignore the assertion in insert and call replace instead . It should be OK to replace old extensions as the replacement will be identical to what's already stored.

The replace function does the same thing insert does but without the assertion (which we should be able to safely ignore):
https://docs.rs/tracing-subscriber/0.2.15/src/tracing_subscriber/registry/extensions.rs.html#90-92

Previously `ExtensionsMut::insert` was used without a
guarantee that we had exclusive access to a SpanRef's extensions.
This could lead to a panic in the `insert` function in the case where
the extension was inserted concurrently by multiple calls to `eval_ctx`.

By using `replace` the assertion is ignored and it no longer matters
whether we have exclusive access or not to a span in `eval_ctx`.
@rasviitanen
Copy link
Author

rasviitanen commented Feb 1, 2021

@inanna-malick I don't want to put stress you, just tagging to make sure you are aware of this PR. Review in whatever pace is comfortable.

@Fishrock123
Copy link
Contributor

These fixes were merged into the fork(s) I maintain & released:

  • eaze-tracing-distributed 0.2.0-eaze.3
  • eaze-tracing-honeycomb 0.2.1-eaze.3

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.

2 participants