Skip to content

Conversation

owent
Copy link
Member

@owent owent commented Jul 10, 2025

Fixes #3530

Changes

  • It's OK when callback_thread has data race in HttpOperation. So just ignore it.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@Copilot Copilot AI review requested due to automatic review settings July 10, 2025 11:00
@owent owent requested a review from a team as a code owner July 10, 2025 11:00
Copy link

netlify bot commented Jul 10, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 80a9bea
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/68887d08eb63a000081b8c2e

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR suppresses ThreadSanitizer warnings for callback_thread in HttpOperation by introducing a dedicated accessor with TSAN-ignore annotations and memory fences.

  • Add HttpOperationAccessor with OPENTELEMETRY_SANITIZER_NO_THREAD wrappers and memory fences around callback_thread access.
  • Replace direct async_data_->callback_thread reads/writes with HttpOperationAccessor::GetThreadId/SetThreadId.
  • Declare HttpOperationAccessor as a friend in the HttpOperation class header.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
ext/src/http/client/curl/http_operation_curl.cc Introduces HttpOperationAccessor and replaces direct callback_thread accesses with accessor calls.
ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h Adds friend class HttpOperationAccessor; to grant the accessor private access to HttpOperation::AsyncData.
Comments suppressed due to low confidence (2)

ext/src/http/client/curl/http_operation_curl.cc:53

  • [nitpick] It may be helpful to add a brief comment explaining why OPENTELEMETRY_SANITIZER_NO_THREAD is applied here, clarifying the intent to silence TSAN warnings for this accessor.
  OPENTELEMETRY_SANITIZER_NO_THREAD static std::thread::id GetThreadId(

ext/src/http/client/curl/http_operation_curl.cc:360

  • Consider adding unit tests that verify callback_thread is correctly set, read, and reset through the accessor functions to prevent regressions in callback synchronization.
        if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id())

{
namespace curl
{

Copy link

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider placing HttpOperationAccessor in an anonymous namespace since it’s only used within this translation unit, to avoid exporting an internal utility.

Suggested change
namespace
{

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anonymous namespace will break the friend access.

Copy link

codecov bot commented Jul 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.00%. Comparing base (ac1172e) to head (80a9bea).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3531      +/-   ##
==========================================
- Coverage   90.03%   90.00%   -0.02%     
==========================================
  Files         220      220              
  Lines        7069     7069              
==========================================
- Hits         6364     6362       -2     
- Misses        705      707       +2     

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch.

See some questions on the ifdef logic.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marcalff marcalff merged commit ba38077 into open-telemetry:main Jul 29, 2025
70 checks passed
@marcalff marcalff changed the title Fixes tsan warnings [EXPORTER] Fixes tsan warnings Jul 29, 2025
nikhilbhatia08 pushed a commit to nikhilbhatia08/opentelemetry-cpp that referenced this pull request Aug 7, 2025
@owent owent deleted the fixes_tsan_with_clang_20 branch August 15, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TSAN] ThreadSanitizer: data race with clang 20 and libc++

3 participants