[ETW] Fix infinite loop in ETWProvider::close#3827
[ETW] Fix infinite loop in ETWProvider::close#3827marcalff merged 3 commits intoopen-telemetry:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical infinite loop bug in the ETWProvider::close() method identified in issue #3815. The bug occurred when closing a provider handle that wasn't the first entry in the provider map, causing the iterator to never advance and the while loop to hang indefinitely. Additionally, the fix adds protection against refCount underflow when close is called too many times on the same handle.
Changes:
- Fixed infinite loop by adding missing iterator increment (++it) in the while loop
- Added refCount underflow protection to prevent negative refCount values
- Added unit test to verify the infinite loop fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| exporters/etw/include/opentelemetry/exporters/etw/etw_provider.h | Fixed missing iterator increment and added refCount underflow check in close() method |
| exporters/etw/test/etw_provider_test.cc | Added test case to verify closing multiple providers doesn't cause infinite loop |
| etw.close(handle2); | ||
| etw.close(handle1); |
There was a problem hiding this comment.
The test should include assertions to verify that the close operations succeeded. Currently, the test only verifies that it doesn't hang, but doesn't check if the operations completed successfully. Consider adding assertions to check the return values and provider registration status.
| TEST(ETWProvider, CheckCloseInfiniteLoop) | ||
| { | ||
| std::string providerName1 = "Provider1"; | ||
| std::string providerName2 = "Provider2"; | ||
|
|
||
| static ETWProvider etw; | ||
| auto handle1 = etw.open(providerName1.c_str()); | ||
| auto handle2 = etw.open(providerName2.c_str()); | ||
|
|
||
| // This should not hang | ||
| etw.close(handle2); | ||
| etw.close(handle1); | ||
| } |
There was a problem hiding this comment.
The test doesn't cover the refCount underflow scenario mentioned in the issue. The fix added a check at lines 213-216 to prevent underflow when close is called too many times on the same handle, but this behavior is not tested. Consider adding a test case that calls close multiple times on the same handle and verifies it returns STATUS_ERROR on subsequent calls.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3827 +/- ##
==========================================
+ Coverage 89.95% 89.96% +0.02%
==========================================
Files 225 225
Lines 7170 7170
==========================================
+ Hits 6449 6450 +1
+ Misses 721 720 -1 🚀 New features to boost your workflow:
|
Fixes #3815
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes