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

Phase 2 of enabling new Stream Frame: Reliable Reset #3817

Merged
merged 128 commits into from
Sep 30, 2023

Conversation

ProjectsByJackHe
Copy link
Contributor

@ProjectsByJackHe ProjectsByJackHe commented Aug 16, 2023

Description

Reliable Reset is a new QUIC extension in the works with a draft RFC: https://datatracker.ietf.org/doc/draft-ietf-quic-reliable-stream-reset/.

This PR implements the current spec as outlined by the IETF. This includes implementing the new CLOSE_STREAM frame, along with the necessary metadata changes, and updates to the SEND and RECV paths to ensure sender / receiver complies with the protocol extension.

The API changes involves adding a new SetParam the user can configure, called ReliableOffsetSend, which determines the minimum amount of bytes that must be delivered before a stream can be aborted.

Testing

A new Data Test was added to check the behavior of the API.
A new Events Test was added to validate the receipt of events upon a reliable reset.
A new API test was added to check the behavior of the Get/Set Parameters.

Documentation

Documentation in Settings.md and StreamShutdown.md was added to outline usage of the preview feature.

@ProjectsByJackHe ProjectsByJackHe self-assigned this Aug 16, 2023
@ProjectsByJackHe ProjectsByJackHe requested a review from a team as a code owner August 16, 2023 18:55
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #3817 (9a2318e) into main (649f971) will decrease coverage by 0.46%.
The diff coverage is 79.86%.

@@            Coverage Diff             @@
##             main    #3817      +/-   ##
==========================================
- Coverage   86.84%   86.38%   -0.46%     
==========================================
  Files          56       56              
  Lines       16712    16854     +142     
==========================================
+ Hits        14513    14560      +47     
- Misses       2199     2294      +95     
Files Coverage Δ
src/core/connection.c 81.46% <ø> (-0.36%) ⬇️
src/core/loss_detection.c 89.84% <100.00%> (+0.06%) ⬆️
src/core/send.c 88.70% <ø> (-0.17%) ⬇️
src/core/send.h 87.93% <ø> (ø)
src/core/sent_packet_metadata.c 100.00% <100.00%> (ø)
src/core/sent_packet_metadata.h 100.00% <ø> (ø)
src/core/stream_set.c 94.58% <ø> (ø)
src/core/frame.c 87.32% <60.00%> (-0.22%) ⬇️
src/core/stream.h 92.03% <75.00%> (-1.30%) ⬇️
src/core/stream_send.c 95.12% <90.47%> (-0.19%) ⬇️
... and 2 more

... and 8 files with indirect coverage changes

src/inc/msquic.h Outdated Show resolved Hide resolved
src/core/stream.c Outdated Show resolved Hide resolved
src/core/stream.c Outdated Show resolved Hide resolved
src/core/stream.c Outdated Show resolved Hide resolved
src/core/stream.h Outdated Show resolved Hide resolved
src/inc/msquic.h Outdated Show resolved Hide resolved
src/core/stream.c Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
ProjectsByJackHe and others added 9 commits September 27, 2023 10:43
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
src/core/stream_recv.c Outdated Show resolved Hide resolved
src/core/stream_recv.c Outdated Show resolved Hide resolved
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

Just a few nits, but otherwise good to go!

src/test/MsQuicTests.h Outdated Show resolved Hide resolved
src/test/MsQuicTests.h Outdated Show resolved Hide resolved
src/test/MsQuicTests.h Outdated Show resolved Hide resolved
src/test/bin/winkernel/control.cpp Outdated Show resolved Hide resolved
ProjectsByJackHe and others added 4 commits September 29, 2023 12:49
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
@nibanks
Copy link
Member

nibanks commented Sep 30, 2023

Going to merge this, but I did notice one test failure:

[ RUN      ] Misc/WithAbortiveArgs.AbortiveShutdown/502
D:\a\msquic\msquic\src\test\lib\DataTest.cpp(1197): error: Client failed to get connected before timeout!

[  FAILED  ] Misc/WithAbortiveArgs.AbortiveShutdown/502, where GetParam() = v6/0/1/0/0/1/2/1/1/0 (3274 ms)
[----------] 1 test from Misc/WithAbortiveArgs (3275 ms total)

This test is (rarely) flaky, so hopefully it's just that, but we need to keep an eye out on this going forward. @ProjectsByJackHe can you please open a bug and attach the logs for this failure to track?

https://github.com/microsoft/msquic/actions/runs/6356433339/job/17266401639?pr=3817

@nibanks nibanks merged commit 0ba1fc7 into main Sep 30, 2023
397 of 400 checks passed
@nibanks nibanks deleted the jackhe/phase2-issue-3775 branch September 30, 2023 13: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 Area: Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants