Datadog Report

Branch report: dario.castane/finish-once
Commit report: 9bdee17
Test service: dd-trace-go

✅ 0 Failed, 4474 Passed, 66 Skipped, 27m 14.85s Total Time

Benchmarks

Benchmark execution time: 2025-03-25 18:27:02

Comparing candidate commit 179f955 in PR branch dario.castane/finish-once with baseline commit 8cc7cca in branch v2-dev.

Found 2 performance improvements and 0 performance regressions! Performance is the same for 54 metrics, 0 unstable metrics.

scenario:BenchmarkSetTagString-24

• 🟩 execution_time [-9.893ns; -6.947ns] or [-8.199%; -5.757%]

scenario:BenchmarkSetTagStringer-24

• 🟩 execution_time [-11.860ns; -9.000ns] or [-7.825%; -5.938%]

I am not sure if this is a complete fix. Everything in s.Finish assumes that s.Lock must be held when modifying s. However, we're still allowing serializeSpanEvents and serializeSpanLinksInMeta to modify s.meta without holding the lock. I.e. a call to s.SetTag could still clash with these operations and cause crashes.

Why didn't we consider acquiring the s.Lock inside of serializeSpanEvents and serializeSpanLinksInMeta? TBH, I think all of the span link and event functions (s.AddLink and s.AddEvent) are prone to race conditions right now. We should consider rolling back those PRs or doing a deep fix/audit ASAP.

@felixge The guard should be enough. It's an atomic value. Two concurrent calls to s.Finish should end with one going forward and the other returning.

Regarding considering acquiring the s.Lock, we tried that but it caused deadlocks. We tried to fix them, but found a roadblock on sampling code that was too complex - required a large refactor - to fix properly in a short span.

The guard should be enough.

The guard is only enough for the double Finish issue. It's not enough for a Finish call concurrent with a SetTag call (or a number of other span modifying calls). The span API is designed to be concurrency safe even under incorrect usage. This assumption no longer holds in various places right now. This is very dangerous IMO.

You’re right to raise this concern. We worked on this together with @darccio and evaluated a few different approaches. In the end, we opted to keep the changes minimal and targeted to avoid introducing broader side effects.

That said, we agree this isn’t a complete fix. The current implementation does have inconsistencies around locking, especially in how nested functions operate on shared state. It’s becoming increasingly difficult to track which paths require s.Lock, and the recent incident clearly highlighted the risks.

We should prioritize a deeper audit of the span code—especially AddEvent, AddLink, and serialization logic—and move toward a more consistent and maintainable locking strategy.