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

tracer: unaligned atomic operation causes panic #1418

Closed
DylanRJohnston-FZ opened this issue Aug 10, 2022 · 1 comment · Fixed by #1443
Closed

tracer: unaligned atomic operation causes panic #1418

DylanRJohnston-FZ opened this issue Aug 10, 2022 · 1 comment · Fixed by #1443
Assignees
@knusbaum
Labels
ack bug unintended behavior that has to be fixed
Milestone
1.42.0

Comments

@DylanRJohnston-FZ
Copy link

I believe this to be related to the issue here DataDog/datadog-go#260, as you can see from these logs,
image ddtrace panics here

dd-trace-go/ddtrace/tracer/spancontext.go

Line 212 in 03a3099

atomic.CompareAndSwapInt64((*int64)(&t.samplingDecision), int64(decisionNone), int64(decisionKeep))
because

dd-trace-go/ddtrace/tracer/spancontext.go

Line 164 in 03a3099

samplingDecision samplingDecision // samplingDecision indicates whether to send the trace to the agent.
is unaligned.

This is running in GCP on an n2 node.
@ajgajg1134
Copy link
Contributor

Hello! Thanks for pointing this out to us. Elsewhere in datadog we've been moving to standardize on go.uber.com/atomic which should prevent these alignment errors. I've made an internal bug ticket for us to fix this.

@ajgajg1134 ajgajg1134 added the ack label Aug 16, 2022
knusbaum added a commit that referenced this issue Aug 23, 2022
@knusbaum
ddtrace/tracer: switch to go.uber.org/atomic
fb24699 
sync/atomic has several issues. Among them is that it causes a panic when a
field isn't correctly aligned. Alignment must me manually ensured and is
easy to forget.

Instead, we will use go.uber.org/atomic which provides wrappers around Go's
standard library atomics. The wrappers ensure we always use atomic
operations to load and store the values, and also prevent the alignment
issue.

Fixes #1418
@knusbaum knusbaum mentioned this issue Aug 23, 2022
Merged
knusbaum added a commit that referenced this issue Aug 23, 2022
@knusbaum
ddtrace/tracer: switch to go.uber.org/atomic
2b1ba99 
sync/atomic has several issues. Among them is that it causes a panic when a
field isn't correctly aligned. Alignment must be manually ensured and is
easy to forget.

Instead, we will use go.uber.org/atomic which provides wrappers around Go's
standard library atomics. The wrappers ensure we always use atomic
operations to load and store the values, and also prevent the alignment
issue.

Fixes #1418
@katiehockman katiehockman changed the title Panic: Unaligned atomic operation tracer: unaligned atomic operation causes panic Sep 2, 2022
@katiehockman katiehockman self-assigned this Sep 2, 2022
@katiehockman katiehockman added the bug unintended behavior that has to be fixed label Sep 2, 2022
@katiehockman katiehockman added this to the 1.42.0 milestone Sep 2, 2022
@katiehockman katiehockman assigned knusbaum and unassigned katiehockman Sep 2, 2022
knusbaum added a commit that referenced this issue Sep 13, 2022
@knusbaum
ddtrace/tracer: switch atomics to 32-bit
bcd6cc1 
sync/atomic has several issues. Among them is that it causes a panic when a
64-bit field isn't correctly aligned. Alignment must be manually ensured
and is easy to forget.

Instead, we will use 32-bit atomic integers which do not require manual
alignment. We can eventually trade them out for Go's new atomics APIs that
were introduced in go1.19, but we have to wait until 1.18 falls out of our
supported versions.

Fixes #1418
knusbaum added a commit that referenced this issue Sep 15, 2022
@knusbaum
ddtrace/tracer: switch atomics to 32-bit (#1443)
3e927ae 
sync/atomic has several issues. Among them is that it causes a panic when
a 64-bit field isn't correctly aligned. Alignment must be manually
ensured and is easy to forget.

Instead, we will use 32-bit atomic integers which do not require manual
alignment. We can eventually trade them out for Go's new atomics APIs
that were introduced in go1.19, but we have to wait until 1.18 falls out
of our supported versions.

Fixes #1418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@knusbaum knusbaum

Labels
ack bug unintended behavior that has to be fixed
Projects
None yet
Milestone
1.42.0
Development

Successfully merging a pull request may close this issue.

ddtrace/tracer: switch atomics to 32-bit DataDog/dd-trace-go
4 participants
@knusbaum @ajgajg1134 @katiehockman @DylanRJohnston-FZ