Fix thread-safety issue in LogRecordSharedPool.Rent()#6833
Conversation
Fixes open-telemetry#6233 - Change TryRentCoreRare failure handling from 'continue' to 'break' to prevent duplicate LogRecord rentals under high contention - Optimize slotIndex calculation - Add regression test RentShouldNeverReturnSameInstanceConcurrently
|
|
|
Hi. @alanwest @open-telemetry/dotnet-approvers could you please review this? It addresses the thread-safety issue in LogRecordSharedPool (Fixes #6233). @TimothyMothra, tagging you as well since this is in the Logs/LogRecord pool area. Also, this is my first time requesting a review in this repo—if I’m not tagging the right people or there’s a preferred process, please let me know and I’ll follow it. |
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
martincostello
left a comment
There was a problem hiding this comment.
Please update the CHANGELOG with the bug fix details.
- Add CHANGELOG entry for thread-safety fix in LogRecordSharedPool.Rent() - Use ConcurrentQueue for duplicate detection messages in test to avoid race condition when collecting failure details"
Co-authored-by: Martin Costello <martin@martincostello.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6833 +/- ##
==========================================
+ Coverage 86.35% 87.28% +0.92%
==========================================
Files 263 263
Lines 12387 12388 +1
==========================================
+ Hits 10697 10813 +116
+ Misses 1690 1575 -115
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
|
Hi @martincostello — re-requesting review 🙇 |
Fixes #6233
Changes
This PR fixes a thread-safety issue in
LogRecordSharedPool.Rent()where the same LogRecord instance could be rented to multiple threads concurrently under high contention.continueand retry with a new index, which could lead to a duplicate rental.slotIndexcalculation to avoid redundant modulo operations.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes