Streams Hold References on Connections

[nibanks]

Description

Make sure to keep the connection alive as long as the stream.

Testing

CI/CD

Documentation

N/A

[codecov]

Codecov Report

Merging #3931 (2034f03) into main (81858e4) will decrease coverage by 0.02%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

@@            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     

Files	Coverage Δ
src/core/connection.c	81.76% <100.00%> (+0.17%)	⬆️
src/core/connection.h	97.23% <ø> (-1.66%)	⬇️
src/core/stream.c	89.79% <100.00%> (-1.00%)	⬇️

... and 17 files with indirect coverage changes

[ManickaP]

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 ConnectionClose call until the count is 0.

[nibanks]

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 ConnectionClose call until the count is 0.

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.

[ManickaP]

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.

[ProjectsByJackHe]

All tests pass except for a handshake test, but its testing using some randomness and this PR doesn't directly impact handshakes, so LGTM!