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.

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

[EXPORTER] OTLP file: use thread-safe file/io #2675

Merged
merged 7 commits into from
Jun 8, 2024

Conversation

owent
Copy link
Member

@owent owent commented May 21, 2024

Fixes #2674

Changes

  • Use fwrite, fflush to keep it thread-safety.
  • Add OPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUND , OPENTELEMETRY_SANITIZER_NO_MEMORY , OPENTELEMETRY_SANITIZER_NO_THREAD, OPENTELEMETRY_SANITIZER_NO_ADDRESS, OPENTELEMETRY_HAVE_BUILTIN, OPENTELEMETRY_HAVE_FEATURE, OPENTELEMETRY_HAVE_ATTRIBUTE, OPENTELEMETRY_HAVE_CPP_ATTRIBUTE

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

@owent owent requested a review from a team May 21, 2024 08:14
Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.67%. Comparing base (497eaf4) to head (93ec062).
Report is 77 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2675      +/-   ##
==========================================
+ Coverage   87.12%   87.67%   +0.55%     
==========================================
  Files         200      190      -10     
  Lines        6109     5851     -258     
==========================================
- Hits         5322     5129     -193     
+ Misses        787      722      -65     
Files Coverage Δ
api/include/opentelemetry/nostd/string_view.h 97.96% <100.00%> (ø)

... and 75 files with indirect coverage changes

owent added 2 commits May 23, 2024 10:52
Add `OPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUND` , `OPENTELEMETRY_SANITIZER_NO_MEMORY` , `OPENTELEMETRY_SANITIZER_NO_THREAD`, `OPENTELEMETRY_SANITIZER_NO_ADDRESS`, `OPENTELEMETRY_HAVE_BUILTIN`, `OPENTELEMETRY_HAVE_FEATURE`, `OPENTELEMETRY_HAVE_ATTRIBUTE`, `OPENTELEMETRY_HAVE_CPP_ATTRIBUTE`
@marcalff marcalff changed the title Use fopen, fwrite, fflush and fclose which are thread-safety. Use fopen, fwrite, fflush and fclose which are thread-safe. May 28, 2024
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 PR.

The macros and annotations for sanitizers look good to me.

For the thread safety issue with file I/O,
I don't understand the initial problem, and the precise root cause.

In my understanding:

  • file iostream operations are thread safe, when each thread uses its own stream
  • file iostream operations are not thread safe when multiple threads uses the same stream
  • file io operations are thread safe, when each thread uses its own FILE handle
  • file io operations are not thread safe (in theory), when multiple threads uses the same FILE handle. In practive, they may appear to be safe if the implementation serializes calls.

Changing iostream to file io does not look like a proper fix for thread safety:

  • If several threads can write to the same handle, do we need a mutex to serialize operations to write completely a record then ?

In particular, with:

  fwrite(data.data(), 1, data.size(), out.get());
  fputc('\n', out.get());

This does not look safe, even when fwrite() and fputc() are safe, if there are really several threads executing this code concurrently for the same handle (both records will be intertwined: 2 data, then 2 end of line).

Changing file stream with plain file io may be ok (even desirable) otherwise, but I don't understand how this is a fix for thread safety: please clarify.

@owent
Copy link
Member Author

owent commented May 31, 2024

  • file iostream operations are not thread safe when multiple threads uses the same stream

file iostream operations are not thread safe when multiple threads uses the same stream(we got data race from std::basic_streambuf<CharT,Traits>::xsputn when using tsan). But fwrite and fflush are thread safe when multiple threads uses the same FILE handle.

You are right, fwrite(data.data(), 1, data.size(), out.get()); and fputc('\n', out.get()); still have concurrency problems here. I have move the codes to append EOL in the caller.

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, thanks for the fix.

@marcalff marcalff changed the title Use fopen, fwrite, fflush and fclose which are thread-safe. [EXPORTER] OTLP file: use fopen, fwrite, fflush and fclose which are thread-safe. May 31, 2024
@esigo esigo self-assigned this Jun 3, 2024
@marcalff
Copy link
Member

marcalff commented Jun 7, 2024

@esigo Hi Ehsan. Any comments for the code review ?

Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM
thanks for the PR :)

@marcalff marcalff merged commit 3efd3ce into open-telemetry:main Jun 8, 2024
50 checks passed
@marcalff marcalff changed the title [EXPORTER] OTLP file: use fopen, fwrite, fflush and fclose which are thread-safe. [EXPORTER] OTLP file: use thread-safe file/io Jun 20, 2024
@owent owent deleted the fix_tsan_for_otlp_file_exporter branch June 21, 2024 06:19
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.

OtlpFileSystemBackend has ThreadSanitizer problems.
3 participants