opentelemetry-cpp
                                
                                 opentelemetry-cpp copied to clipboard
                                
                                    opentelemetry-cpp copied to clipboard
                            
                            
                            
                        [SDK] Fix forceflush may wait for ever
Fixes #2574
May also fixes #2583
Changes
- Using a sequence to check if a complete notification includes the datas when call ForceFlush.
For significant contributions please make sure you have completed the following items:
- [ ] CHANGELOG.mdupdated for non-trivial changes
- [x] Unit tests have been added
- [x] Changes in public API reviewed
@ThomsonTan Is there any problem in this PR? Maybe we can close #2553 if this PR is merged. It fixes the same problems as #2553 do and also fix the simular problems in trace and logs.
BTW, can you explain in brief that why the sequence notification can break the potential forever wait? Thanks.
BTW, can you explain in brief that why the sequence notification can break the potential forever wait? Thanks.
When more than one threads call ForceFlush concurrency, is_force_flush_notified may be set true only once in background thread and one of the threads call ForceFlush will wait is_force_flush_notified for ever.
Codecov Report
Attention: Patch coverage is 95.94595% with 3 lines in your changes are missing coverage. Please review.
Project coverage is 87.70%. Comparing base (
497eaf4) to head (d3df9a4). Report is 65 commits behind head on main.
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2584      +/-   ##
==========================================
+ Coverage   87.12%   87.70%   +0.58%     
==========================================
  Files         200      190      -10     
  Lines        6109     5852     -257     
==========================================
- Hits         5322     5132     -190     
+ Misses        787      720      -67     
| Files | Coverage Δ | |
|---|---|---|
| ...pentelemetry/sdk/logs/batch_log_record_processor.h | 100.00% <100.00%> (ø) | |
| ...ude/opentelemetry/sdk/trace/batch_span_processor.h | 100.00% <100.00%> (ø) | |
| sdk/src/logs/batch_log_record_processor.cc | 89.21% <96.16%> (+1.71%) | :arrow_up: | 
| ...metrics/export/periodic_exporting_metric_reader.cc | 81.32% <95.00%> (+1.98%) | :arrow_up: | 
| sdk/src/trace/batch_span_processor.cc | 94.08% <96.16%> (+1.65%) | :arrow_up: | 
Adding ok-to-merge.
Waiting to give @lalitb and @esigo a chance to comment, and planning to merge early next week.