Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5892 +/- ##
==========================================
+ Coverage 84.73% 85.65% +0.91%
==========================================
Files 60 60
Lines 18751 18754 +3
==========================================
+ Hits 15889 16064 +175
+ Misses 2862 2690 -172 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
It would be nice to also add a call to StreamProvideReceiveBuffers when a new stream is notified to a connection.
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
Looks largely good, but the failures in the CI are the sign of either a bug in MsQuic the new test is finding, or that the spintest is doing something wrong.
Please address it before we merge this PR (in a different PR if the issue is a MsQuic bug)
Pulling the latest changes seem to have removed the issue. Maybe due to the latest xdp removal effort. |
| // connection count bounded. Otherwise teardown blows the | ||
| // watchdog under Debug + alloc_fail. |
There was a problem hiding this comment.
Why? That doesn't seem obvious, and we should probably fix the teardown if it can take that long?
There was a problem hiding this comment.
The dumps show three spin_client threads simultaneously inside RtlDebugFreeHeap, specifically one of them blocked on RtlEnterCriticalSectionContended waiting for the heap CS, while the other two walk free lists. Every free() on the Windows debug heap takes a per-heap critical section, so N concurrent closing connections serialize on it
3 participants
Description
SpinQuic updated to cover the new/experimental APIs:
ConnectionPoolCreate
RegistrationClose2
App-provided receive buffers APIs: StreamProvideReceiveBuffers_Describe the purpose of and changes within this Pull Request._
Testing
Do any existing tests cover this change? Are new tests needed?
Documentation
Is there any documentation impact for this change?