Skip to content

Conversation

@cijothomas
Copy link
Member

If shutdown is called from multiple threads, modify the logic so that only one enters the core logic of sending shutdown signal etc. Previously, both threads can send shutdown message to background thread, and after the 1st one, the thread exists, so the other shutdown fails. Though it is okay, the error message may not be super easy to follow.
This PR makes sure only one shutdown proceeds, the other one immediately returning a failure indicating that failure is due to shutdown already initiated. This also ensures, checking for shutdown flag is only done at user thread invoking shutdown/flush.

There is a similar race between Flush and Shutdown possible, added it as a TODO to improve log messaging.

Also added more test for drop.

@cijothomas cijothomas requested a review from a team as a code owner December 13, 2024 02:36
@codecov
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.3%. Comparing base (9b0ccce) to head (21bbd6b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-sdk/src/metrics/periodic_reader.rs 92.1% 3 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2422   +/-   ##
=====================================
  Coverage   79.3%   79.3%           
=====================================
  Files        122     122           
  Lines      21565   21588   +23     
=====================================
+ Hits       17110   17134   +24     
+ Misses      4455    4454    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

Left some suggestions.

@cijothomas cijothomas merged commit 15d69b1 into open-telemetry:main Dec 13, 2024
19 checks passed
@cijothomas cijothomas deleted the cijothomas/periodicreader-fix3 branch December 13, 2024 15:56
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.

3 participants