fix(profiling): fix data race when accessing span for thread #11167

nsrip-dd · 2024-10-25T00:44:19Z

The ThreadSpanLinks singleton holds the active span (if one exists) for
a given thread ID. The get_active_span_from_thread_id member function
returns a pointer to the active span for a thread. The link_span member
function sets the active span for a thread. get_active_span_from_thread_id
accesses the map of spans under a mutex, but returns the pointer after
releasing the mutex, meaning link_span can modify the members of the
Span while the caller of get_active_span_from_thread_id is reading them.

Fix this by returning a copy of the Span. Use a std::optional to wrap
the return value of get_active_span_from_thread_id, rather than
returning a pointer. We want to tell whether or not there actually was a
span associated with the thread, but returning a pointer would require
us to heap allocate the copy of the Span.

I added a simplistic regression test which fails reliably without this fix
when built with the thread sanitizer enabled. Output like:

WARNING: ThreadSanitizer: data race (pid=2971510)
  Read of size 8 at 0x7b2000004080 by thread T2:
    #0 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:823 (libtsan.so.0+0x42313)
    #1 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:815 (libtsan.so.0+0x42313)
    #2 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_assign(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) <null> (libstdc++.so.6+0x1432b4)
    #3 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > std::__invoke_impl<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()>(std::__invoke_other, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*&&)()) <null> (thread_span_links+0xe46e)
    #4 std::__invoke_result<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()>::type std::__invoke<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*&&)()) <null> (thread_span_links+0xe2fe)
    #5 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > std::thread::_Invoker<std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) <null> (thread_span_links+0xe1cf)
    #6 std::thread::_Invoker<std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()> >::operator()() <null> (thread_span_links+0xe0f6)
    #7 std::thread::_State_impl<std::thread::_Invoker<std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()> > >::_M_run() <null> (thread_span_links+0xdf40)
    #8 <null> <null> (libstdc++.so.6+0xd6df3)

  Previous write of size 8 at 0x7b2000004080 by thread T1 (mutexes: write M47):
    #0 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:823 (libtsan.so.0+0x42313)
    #1 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:815 (libtsan.so.0+0x42313)
    #2 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_assign(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocato
r<char> > const&) <null> (libstdc++.so.6+0x1432b4)
    #3 get() <null> (thread_span_links+0xb570)
    #4 void std::__invoke_impl<void, void (*)()>(std::__invoke_other, void (*&&)()) <null> (thread_span_links+0xe525)
    #5 std::__invoke_result<void (*)()>::type std::__invoke<void (*)()>(void (*&&)()) <null> (thread_span_links+0xe3b5)
    #6 void std::thread::_Invoker<std::tuple<void (*)()> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) <null> (thread_span_links+0xe242)
    #7 std::thread::_Invoker<std::tuple<void (*)()> >::operator()() <null> (thread_span_links+0xe158)
[ ... etc ... ]

Checklist

PR author has checked that all the criteria below are met
The PR description includes an overview of the change
The PR description articulates the motivation for the change
The change includes tests OR the PR description describes a testing strategy
The PR description notes risks associated with the change, if any
Newly-added code is easy to change
The change follows the library release note guidelines
The change includes or references documentation updates if necessary
Backport labels are set (if applicable)

Reviewer Checklist

Reviewer has checked that all the criteria below are met
Title is accurate
All changes are related to the pull request's stated goal
Avoids breaking API changes
Testing strategy adequately addresses listed risks
Newly-added code is easy to change
Release note makes sense to a user of the library
If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
Backport labels are set in a manner that is consistent with the release branch maintenance policy

The ThreadSpanLinks singleton holds the active span (if one exists) for a given thread ID. The get_active_span_from_thread_id member function returns a pointer to the active span for a thread. The link_span member function sets the active span for a thread. get_active_span_from_thread_id accesses the map of spans under a mutex, but returns the pointer after releasing the mutex, meaning link_span can modify the members of the Span while the caller of get_active_span_from_thread_id is reading them. Fix this by returning a copy of the Span. Use a std::optional to wrap the return value of get_active_span_from_thread_id, rather than returning a pointer. We want to tell whether or not there actually was a span associated with the thread, but returning a pointer would require us to heap allocate the copy of the Span.

github-actions · 2024-10-25T00:44:51Z

CODEOWNERS have been resolved as:

ddtrace/internal/datadog/profiling/stack_v2/test/CMakeLists.txt         @DataDog/profiling-python
ddtrace/internal/datadog/profiling/stack_v2/test/thread_span_links.cpp  @DataDog/profiling-python
releasenotes/notes/profiling-fix-data-race-segfault-b14f59c06c6bf9ed.yaml  @DataDog/apm-python
ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt              @DataDog/profiling-python
ddtrace/internal/datadog/profiling/stack_v2/include/thread_span_links.hpp  @DataDog/profiling-python
ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp      @DataDog/profiling-python
ddtrace/internal/datadog/profiling/stack_v2/src/thread_span_links.cpp   @DataDog/profiling-python

pr-commenter · 2024-10-25T01:24:24Z

Benchmarks

Benchmark execution time: 2024-10-29 16:29:25

Comparing candidate commit 0586972 in PR branch nick.ripley/fix-stackv2-span-link-data-race with baseline commit ae34e92 in branch main.

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

The test is pretty simplistic, just creates new span_type strings and links them to a "thread" in one thread, and reads out the span_type for that "thread" in another thread. This fails reliably for me when built with the thread sanitizer enabled without the data race fix, and passes reliably with the fix.

…v2-span-link-data-race

ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt

github-actions · 2024-10-29T16:37:58Z

The backport to 2.12 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.12 2.12
# Navigate to the new working tree
cd .worktrees/backport-2.12
# Create a new branch
git switch --create backport-11167-to-2.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 64b33747062c585df9f0eb810c28d367379a9998
# Push it to GitHub
git push --set-upstream origin backport-11167-to-2.12
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.12

Then, create a pull request where the base branch is 2.12 and the compare/head branch is backport-11167-to-2.12.

github-actions · 2024-10-29T16:37:59Z

The backport to 2.13 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.13 2.13
# Navigate to the new working tree
cd .worktrees/backport-2.13
# Create a new branch
git switch --create backport-11167-to-2.13
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 64b33747062c585df9f0eb810c28d367379a9998
# Push it to GitHub
git push --set-upstream origin backport-11167-to-2.13
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.13

Then, create a pull request where the base branch is 2.13 and the compare/head branch is backport-11167-to-2.13.

github-actions · 2024-10-29T16:37:59Z

The backport to 2.14 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.14 2.14
# Navigate to the new working tree
cd .worktrees/backport-2.14
# Create a new branch
git switch --create backport-11167-to-2.14
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 64b33747062c585df9f0eb810c28d367379a9998
# Push it to GitHub
git push --set-upstream origin backport-11167-to-2.14
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.14

Then, create a pull request where the base branch is 2.14 and the compare/head branch is backport-11167-to-2.14.

github-actions · 2024-10-29T16:38:00Z

The backport to 2.15 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.15 2.15
# Navigate to the new working tree
cd .worktrees/backport-2.15
# Create a new branch
git switch --create backport-11167-to-2.15
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 64b33747062c585df9f0eb810c28d367379a9998
# Push it to GitHub
git push --set-upstream origin backport-11167-to-2.15
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.15

Then, create a pull request where the base branch is 2.15 and the compare/head branch is backport-11167-to-2.15.

The ThreadSpanLinks singleton holds the active span (if one exists) for a given thread ID. The `get_active_span_from_thread_id` member function returns a pointer to the active span for a thread. The `link_span` member function sets the active span for a thread. `get_active_span_from_thread_id` accesses the map of spans under a mutex, but returns the pointer after releasing the mutex, meaning `link_span` can modify the members of the Span while the caller of `get_active_span_from_thread_id` is reading them. Fix this by returning a copy of the `Span`. Use a `std::optional` to wrap the return value of `get_active_span_from_thread_id`, rather than returning a pointer. We want to tell whether or not there actually was a span associated with the thread, but returning a pointer would require us to heap allocate the copy of the Span. I added a simplistic regression test which fails reliably without this fix when built with the thread sanitizer enabled. Output like: ``` WARNING: ThreadSanitizer: data race (pid=2971510) Read of size 8 at 0x7b2000004080 by thread T2: #0 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:823 (libtsan.so.0+0x42313) #1 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:815 (libtsan.so.0+0x42313) #2 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_assign(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) <null> (libstdc++.so.6+0x1432b4) #3 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > std::__invoke_impl<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()>(std::__invoke_other, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*&&)()) <null> (thread_span_links+0xe46e) #4 std::__invoke_result<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()>::type std::__invoke<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*&&)()) <null> (thread_span_links+0xe2fe) #5 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > std::thread::_Invoker<std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) <null> (thread_span_links+0xe1cf) #6 std::thread::_Invoker<std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()> >::operator()() <null> (thread_span_links+0xe0f6) #7 std::thread::_State_impl<std::thread::_Invoker<std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > (*)()> > >::_M_run() <null> (thread_span_links+0xdf40) #8 <null> <null> (libstdc++.so.6+0xd6df3) Previous write of size 8 at 0x7b2000004080 by thread T1 (mutexes: write M47): #0 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:823 (libtsan.so.0+0x42313) #1 memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:815 (libtsan.so.0+0x42313) #2 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_assign(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocato r<char> > const&) <null> (libstdc++.so.6+0x1432b4) #3 get() <null> (thread_span_links+0xb570) #4 void std::__invoke_impl<void, void (*)()>(std::__invoke_other, void (*&&)()) <null> (thread_span_links+0xe525) #5 std::__invoke_result<void (*)()>::type std::__invoke<void (*)()>(void (*&&)()) <null> (thread_span_links+0xe3b5) #6 void std::thread::_Invoker<std::tuple<void (*)()> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) <null> (thread_span_links+0xe242) #7 std::thread::_Invoker<std::tuple<void (*)()> >::operator()() <null> (thread_span_links+0xe158) [ ... etc ... ] ``` (cherry picked from commit 64b3374)

taegyunkim added Profiling Continous Profling backport 2.12 backport 2.13 backport 2.14 backport 2.15 labels Oct 25, 2024

nsrip-dd added 4 commits October 28, 2024 19:52

Merge remote-tracking branch 'origin/main' into nick.ripley/fix-stack…

3717262

…v2-span-link-data-race

chore: clang-format

99c8fd6

chore: add release note

d42afe5

nsrip-dd marked this pull request as ready for review October 29, 2024 15:04

nsrip-dd requested review from a team as code owners October 29, 2024 15:04

nsrip-dd requested review from sanchda and lievan October 29, 2024 15:04

P403n1x87 approved these changes Oct 29, 2024

View reviewed changes

ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt Outdated Show resolved Hide resolved

chore: add trailing newline

0586972

taegyunkim added the backport 2.16 label Oct 29, 2024

taegyunkim approved these changes Oct 29, 2024

View reviewed changes

sanchda approved these changes Oct 29, 2024

View reviewed changes

nsrip-dd merged commit 64b3374 into main Oct 29, 2024
634 of 635 checks passed

nsrip-dd deleted the nick.ripley/fix-stackv2-span-link-data-race branch October 29, 2024 16:37

github-actions bot mentioned this pull request Oct 29, 2024

fix(profiling): fix data race when accessing span for thread [backport 2.16] #11210

Open

2 tasks

nsrip-dd mentioned this pull request Oct 29, 2024

fix(profiling): fix data race when accessing span for thread [backport 2.15] #11211

Open

2 tasks

nsrip-dd mentioned this pull request Oct 29, 2024

fix(profiling): fix data race when accessing span for thread [backport 2.14] #11213

Open

2 tasks

nsrip-dd mentioned this pull request Oct 29, 2024

fix(profiling): fix data race when accessing span for thread [backport 2.13] #11214

Open

2 tasks

nsrip-dd mentioned this pull request Oct 29, 2024

fix(profiling): fix data race when accessing span for thread [backport 2.12] #11215

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(profiling): fix data race when accessing span for thread #11167

fix(profiling): fix data race when accessing span for thread #11167

nsrip-dd commented Oct 25, 2024 •

edited by taegyunkim

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

pr-commenter bot commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

fix(profiling): fix data race when accessing span for thread #11167

fix(profiling): fix data race when accessing span for thread #11167

Conversation

nsrip-dd commented Oct 25, 2024 • edited by taegyunkim Loading

Checklist

Reviewer Checklist

github-actions bot commented Oct 25, 2024 • edited Loading

pr-commenter bot commented Oct 25, 2024 • edited Loading

Benchmarks

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

nsrip-dd commented Oct 25, 2024 •

edited by taegyunkim

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

pr-commenter bot commented Oct 25, 2024 •

edited

Loading