-
Notifications
You must be signed in to change notification settings - Fork 531
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
Streams Hold References on Connections #3931
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3931 +/- ##
==========================================
- Coverage 86.62% 86.60% -0.02%
==========================================
Files 56 56
Lines 16889 16893 +4
==========================================
+ Hits 14630 14631 +1
- Misses 2259 2262 +3
|
Does this mean that we don't have to keep connection alive anymore until all streams are closed? Because we have stream ref counting which holds back |
Not necessarily. At least not with just this PR. We might end up going that direction (especially if you really want/need it), but right now, we're working on solving a very small race edge case where even if you close a stream and then the connection (all on the QUIC callback) while you were calling send on your thread, it's possible for MsQuic to use-after-free. We've only seen this with SMB in extremely rare cases; and we're trying to fix it. FYI, this PR as it stands right now though, is causing deadlocks. 😦 We need to fix this first, but I think once it is fixed, it'd be easy enough to support closing in different orders. |
Oh, I see, thanks for the explanation. We don't need it as we do have a working solution. I just got excited, because it would simplified our code a little. |
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.
All tests pass except for a handshake test, but its testing using some randomness and this PR doesn't directly impact handshakes, so LGTM!
Description
Make sure to keep the connection alive as long as the stream.
Testing
CI/CD
Documentation
N/A