fix(ddtrace/tracer): add finished check on NoDebugStack and WithError code branches#3441
Conversation
Can you give an example? By releasing the lock after checking for finished, the value of finished might be modified by another goroutine calling Finish at the same time. So I don't see how this could provide additional resilience. |
Datadog ReportBranch report: ✅ 0 Failed, 4481 Passed, 64 Skipped, 3m 48.36s Total Time |
BenchmarksBenchmark execution time: 2025-04-16 07:06:28 Comparing candidate commit af71857 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 55 metrics, 1 unstable metrics. |
darccio
changed the title
e9e858e to
590a89a
Compare
darccio
changed the title
| s.RUnlock() | ||
| return | ||
| } | ||
| s.RUnlock() |
There was a problem hiding this comment.
This seems incorrect. Consider two goroutines calling finish concurrency:
G1: Checks `s.finished` and finds it to be false and continues
G2: Checks `s.finished` and finds it to be false and continues
G1: Manages to call `s.finish` setting `finished` to true
Worker: Starts encoding the span
G2: Calls `delete` on `s.meta`
💣
There is no weak version of Invariant I that will do here.
There was a problem hiding this comment.
@darccio We should reduce the number of calls to acquiring lock here to one. It will make things to easier to reason about. This is my current approach with the refactoring work.
3 participants
What does this PR do?
Follow-up from #3435 & #3427. Adds a check on
span.finishedwhen applyingNoDebugStack.Motivation
Adds an extra layer of resilience on high concurrency scenarios.
Reviewer's Checklist
golangci-lint runlocally.Unsure? Have a question? Request a review!