Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Data race when sending spans #38

Closed
blake-wilson opened this issue Nov 21, 2018 · 0 comments
Closed

Data race when sending spans #38

blake-wilson opened this issue Nov 21, 2018 · 0 comments

Comments

@blake-wilson
Copy link

blake-wilson commented Nov 21, 2018 •
edited
Loading

While running some tests with the race detector enabled, we noticed the following data race in the Span's Send method:

Read at 0x00c008aed051 by goroutine 173:
  github.com/{***}/vendor/github.com/honeycombio/beeline-go/trace.(*Span).Send()
      /go/src/github.com/{***}/vendor/github.com/honeycombio/beeline-go/trace/trace.go:241 +0x3f6
  github.com/{***}/vendor/github.com/{***}/finstrument.(*Span).Finish()

Previous write at 0x00c008aed051 by goroutine 212:
  github.com/{***}/vendor/github.com/honeycombio/beeline-go/trace.(*Span).Send()
      /go/src/github.com/{***}/vendor/github.com/honeycombio/beeline-go/trace/trace.go:250 +0x48c
  github.com/{***}/vendor/github.com/{***}/finstrument.(*Span).Finish()

I am using the latest master of libhoney-go (a6839afb6be6f3ec9de43b3a2c08f89b338e8534) and beeline-go (87bb646).

These lines correspond to the following code

It's not clear exactly what happened to me, but perhaps if a parent span is sent at the same time as one of its children it could lead to this race?
tredman added a commit that referenced this issue Nov 23, 2018
@tredman
[trace] Fix race between CreateAsyncChild and span.Send
f5f2674 
`CreateChild` adds a new span to the current span's list of children. `CreateAsyncChild` calls `CreateChild`, then sets the isAsync bool on the span. This can lead to a race caused by simultaneous read of the child span's `isAsync` bool by `Send` as it's being written by `CreateAsyncChild`. To fix this, I refactored  the code a bit to allow us to set `isAsync` before the span is added to the list of child spans.

This doesn't seem to be the same race reported in #38 but it should be fixed either way.
@tredman tredman mentioned this issue Nov 23, 2018
Merged
tredman added a commit that referenced this issue Nov 23, 2018
@tredman
[trace] fix race when child and parent send is called at the same time
b2a7499 
Fixes #38  - if `Send` is called for a span and its parent at the same time, a race can occur when accesing "isSent". This adds a lock to `Send`, which sets `isSent` and protects reads with read locks. This also effectively prevents `Send` from executing multiple times for the same span.
@tredman tredman mentioned this issue Nov 23, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@blake-wilson