Spinquic to use operation priority #4351
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4351 +/- ##
==========================================
- Coverage 86.01% 84.97% -1.04%
==========================================
Files 56 56
Lines 15521 15525 +4
==========================================
- Hits 13351 13193 -158
- Misses 2170 2332 +162 ☔ View full report in Codecov by Sentry. |
|
What! CIFuzz passes!! |
src/core/connection.h
Outdated
| @@ -1067,6 +1067,10 @@ QuicConnRelease( | |||
| QuicConnValidate(Connection); | |||
|
|
|||
| #if DEBUG | |||
| if (Connection->RefTypeCount[Ref] <= 0) { | |||
| fprintf(stderr, "RefTypeCount[%d] is %d\n", Ref, Connection->RefTypeCount[Ref]); | |||
There was a problem hiding this comment.
RefTypeCount[6] is -22829
RefTypeCount is short type (from -32,768 to 32,767). So not just off-by-one issue
There was a problem hiding this comment.
OSS-Fuzz issue dashboard has been listing same issue and the cause seems to be from other PR?
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68176&q=msquic
https://oss-fuzz.com/testcase-detail/4533150459756544
d65bb1a...102a491
There was a problem hiding this comment.
There was a problem hiding this comment.
My guess is that the combination of the that change and priority exposes some bug, most likely in spinquic, for object lifetime. But I have no idea specifically what could be causing it.
ami-GS
force-pushed
the
dev/daiki/spinquic_op_priority
branch
2 times, most recently
from
6bff42e
to
1964ff9
Compare
|
@ami-GS do we still need this PR? |
|
This needs to be merged. Current spinquic doesn't fully utilize priority operation feature |
|
Can you please merge |
nibanks
added
Area: Testing
| @@ -796,6 +796,8 @@ MsQuicStreamStart( | |||
| QUIC_STATUS Status; | |||
| QUIC_STREAM* Stream; | |||
| QUIC_CONNECTION* Connection; | |||
| BOOLEAN IsPriority = !!(Flags & QUIC_STREAM_START_FLAG_PRIORITY_WORK); | |||
| Flags &= ~QUIC_STREAM_START_FLAG_PRIORITY_WORK; | |||
There was a problem hiding this comment.
Why are all these changes necessary?
There was a problem hiding this comment.
I found this is written for other api (StreamSend, SetParam etc.), so I added. Originally to check whether this fix stall issue
Description
Spinquic to use StreamSend, StreamStart and Get/SetParm op priorities.
Testing
See automation
Documentation
N/A