-
Notifications
You must be signed in to change notification settings - Fork 640
Add global Stream tracker in debug builds #5598
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5598 +/- ##
==========================================
+ Coverage 84.81% 85.29% +0.47%
==========================================
Files 60 60
Lines 18647 18658 +11
==========================================
+ Hits 15816 15914 +98
+ Misses 2831 2744 -87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| CxPlatLockInitialize(&MsQuicLib.Lock); | ||
| CxPlatDispatchLockInitialize(&MsQuicLib.DatapathLock); | ||
| #if DEBUG | ||
| CxPlatDispatchLockInitialize(&QuicStreamTrackerLock); |
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.
Nit: I'd suggest simplifying this to one lock for all the trackers.
| // | ||
| // Global stream object tracker for debugging. | ||
| // | ||
| CXPLAT_LIST_ENTRY QuicStreamTrackerList = { &QuicStreamTrackerList, &QuicStreamTrackerList }; |
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.
I've never seen this pattern before - is there a precedent for it in MsQuic? If not, let's use the init routine.
| #if DEBUG | ||
| CXPLAT_DBG_ASSERT(!CxPlatRefDecrement(&Stream->RefTypeBiasedCount[QUIC_STREAM_REF_APP])); | ||
| CxPlatDispatchLockAcquire(&QuicStreamTrackerLock); | ||
| CxPlatListEntryRemove(&Stream->TrackerLink); |
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.
I think we may want to introduce some abstractions for these trackers, such that if perf becomes a practical bottleneck on debug builds, we can:
- Easily add some more fields beyond the list entry
- Easily replace the single global list with something more sophisticated, such as a per-CPU list
|
I am not convinced this helps given that |
The debugger extension is fine, but isn't cross platform. We need a way to provide simple debugging steps for all platforms, so having simple, manually-inspectable lists seems the path of least cost and most reliability. In terms of the stream tracker specifically being redundant, I agree it's perhaps a lower value object if the owning connections can be reliably found. I'd think of this PR more as a litmus test for the general model than a standalone PR that will dramatically change the debugging experience by itself. |
Good point for the debugger extension. It still feels redundant to me, or at least, I have a hard time thinking of a scenario where this global list brings much value over the per-connection list. If there are scenarios, it would be good to extend our trouble shooting guide (or have another doc file focused on debugger) with a list of those debug helpers and when they are relevant. It will be easier to do as we go. |
I stumbled onto a concrete example of why we need the simple, robust object tracker: Lines 335 to 344 in 9117f56
We can see that following the chain of references here by transitivity will fail, more or less by the existing design, and practically necessarily so, if expected object lifetime relationships break down. Since the lifetimes breaking down is exactly one interesting use case, splitting this into separate lists seems best. |
## Description _Describe the purpose of and changes within this Pull Request._ Extend #5598 by rewriting commit authorship (to allow @anrossi to approve this PR) and implementing the suggested refactorings. ## Testing _Do any existing tests cover this change? Are new tests needed?_ Builds locally, CI. ## Documentation _Is there any documentation impact for this change?_ N/A.
Description
Help debug lost reference issues in MsQuic by keeping a global list of streams.
Testing
CI
Documentation
N/A