Skip to content
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

Support Using Streams after Connection Closure #3938

Merged
merged 19 commits into from
Nov 30, 2023

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Oct 25, 2023

Description

Allows apps to not worry about close order between streams and connections.

Testing

New test added. Also leverages existing tests (which found some other issues).

Documentation

TODO?

@nibanks nibanks added Area: API Area: Core Related to the shared, core protocol logic labels Oct 25, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (67ce5af) 87.17% compared to head (d137ad6) 87.01%.
Report is 1 commits behind head on main.

❗ Current head d137ad6 differs from pull request most recent head b7965d6. Consider uploading reports for the commit b7965d6 to get more accurate results

Files Patch % Lines
src/core/connection.c 92.59% 4 Missing ⚠️
src/core/timer_wheel.c 95.34% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3938      +/-   ##
==========================================
- Coverage   87.17%   87.01%   -0.16%     
==========================================
  Files          56       56              
  Lines       16958    16957       -1     
==========================================
- Hits        14783    14755      -28     
- Misses       2175     2202      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shit-lord
Copy link

So currently I can't use stream after its connection closure? I thought the api would reliably return some kind of error code.

@nibanks
Copy link
Member Author

nibanks commented Nov 27, 2023

So currently I can't use stream after its connection closure? I thought the api would reliably return some kind of error code.

Well, once I finish this PR (ETA: this week) then it won't matter, but the problem before was that the connection would be deleted when you close the stream, so any access would be a use-after-free, without adding more state to the stream itself.

@nibanks nibanks marked this pull request as ready for review November 28, 2023 14:06
@nibanks nibanks requested a review from a team as a code owner November 28, 2023 14:06
@nibanks nibanks enabled auto-merge (squash) November 30, 2023 16:00
Copy link
Contributor

@ProjectsByJackHe ProjectsByJackHe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@nibanks nibanks merged commit 6e1c3d6 into main Nov 30, 2023
407 of 409 checks passed
@nibanks nibanks deleted the nibanks/support-stream-after-connection branch November 30, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API Area: Core Related to the shared, core protocol logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants