Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

Context(), Tracer() aren't taking the lock #44

Open
RaduBerinde opened this issue Oct 13, 2016 · 1 comment
Open

Context(), Tracer() aren't taking the lock #44

RaduBerinde opened this issue Oct 13, 2016 · 1 comment

Comments

@RaduBerinde
Copy link
Contributor

func (s *spanImpl) Tracer() opentracing.Tracer {
    return s.tracer
}

func (s *spanImpl) Context() opentracing.SpanContext {
    return s.raw.Context
}

These functions are not taking their locks. Context() could happen concurrently to e.g. a SetBaggageItem call. For Tracer() it's less clear if it should take the lock, as it is immutable except for the DebugAssertUseAfterFinish case:

    if s.tracer.options.DebugAssertUseAfterFinish {
        // This makes it much more likely to catch a panic on any subsequent
        // operation since s.tracer is accessed on every call to `Lock`.
        // We don't call `reset()` here to preserve the logs in the Span
        // which are printed when the assertion triggers.
        s.tracer = nil
    }

It's a question of whether we want data races to show up in that case (in addition to panics). BTW I don't understand the "s.tracer is accessed on every call to Lock" part in that comment, it doesn't seem true.
@tschottdorf

@bhs
Copy link
Contributor

bhs commented Oct 13, 2016

The Context() case is a bug... it was previously a pointer that was immutable during the Span lifetime. The Tracer() case seems ok to me as-is, though. I am being lazy and haven't checked whether Tracer() is called while the lock is held or not...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants