-
Notifications
You must be signed in to change notification settings - Fork 440
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
[EXPORTER] OTLP file: use thread-safe file/io #2675
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
|
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`
fopen
, fwrite
, fflush
and fclose
which are thread-safety.fopen
, fwrite
, fflush
and fclose
which are thread-safe.
There was a problem hiding this 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.
file iostream operations are not thread safe when multiple threads uses the same stream(we got data race from You are right, |
There was a problem hiding this 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.
fopen
, fwrite
, fflush
and fclose
which are thread-safe.fopen
, fwrite
, fflush
and fclose
which are thread-safe.
@esigo Hi Ehsan. Any comments for the code review ? |
There was a problem hiding this 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 :)
fopen
, fwrite
, fflush
and fclose
which are thread-safe.
Fixes #2674
Changes
fwrite
,fflush
to keep it thread-safety.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