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

[QUIC] QuicStream add ReadsCompleted #57492

Merged
merged 6 commits into from
Aug 17, 2021

Conversation

CarnaViire
Copy link
Member

Add ReadsCompleted API that exposes ReadState=ReadsCompleted, set ReadState to ReadsCompleted if FIN flag arrives in RECEIVE event, fix ReadState changing to final stauses, expand ReadState transition description

Fixes #55707

cc @JamesNK @Tratcher

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Aug 16, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Add ReadsCompleted API that exposes ReadState=ReadsCompleted, set ReadState to ReadsCompleted if FIN flag arrives in RECEIVE event, fix ReadState changing to final stauses, expand ReadState transition description

Fixes #55707

cc @JamesNK @Tratcher

Author: CarnaViire
Assignees: -
Labels:

new-api-needs-documentation, area-System.Net.Quic

Milestone: -

@@ -788,5 +789,80 @@ public async Task BigWrite_SmallRead_Success(bool closeWithData)
}
}
}

[Fact]
public async Task BasicTest_WithReadsCompletedCheck()
Copy link
Member

Choose a reason for hiding this comment

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

Unfinished test name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The name's like that because it is basically the same test as QuicStreamTests.BasicTest

public async Task BasicTest()
{
await RunClientServer(
iterations: 100,
serverFunction: async connection =>
{
await using QuicStream stream = await connection.AcceptStreamAsync();
byte[] buffer = new byte[s_data.Length];
int bytesRead = await ReadAll(stream, buffer);
Assert.Equal(s_data.Length, bytesRead);
Assert.Equal(s_data, buffer);
await stream.WriteAsync(s_data, endStream: true);
await stream.ShutdownCompleted();
},
clientFunction: async connection =>
{
await using QuicStream stream = connection.OpenBidirectionalStream();
await stream.WriteAsync(s_data, endStream: true);
byte[] buffer = new byte[s_data.Length];
int bytesRead = await ReadAll(stream, buffer);
Assert.Equal(s_data.Length, bytesRead);
Assert.Equal(s_data, buffer);
await stream.ShutdownCompleted();
}
);
}

But with added ReadsCompleted checks 😄
In your initial commit, you've added these checks to the BasicTest itself. But that makes it non-fully-basic then 😄 and it had flaky problems with Mock behavior. As we don't prioritize fixing Mock, I moved the test with checks to the test collection that only runs on MsQuic.

That being said, I have no strong feeling in sticking to the name BasicTest. So I can rename it to e.g. SendReceive_WithReadsCompletedCheck

Copy link
Member

Choose a reason for hiding this comment

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

Ah. I wasn't familiar with BasicTest so this name seemed non-descript. I'll let you decide what to do with it.

@adityamandaleeka
Copy link
Member

Thanks @CarnaViire!

@karelz
Copy link
Member

karelz commented Aug 17, 2021

Browser runs are hitting #57501, unrelated to this PR.
Libraries test failures are test suites which were cut off (similar things are happening on other PRs). None of the test suites are related to Quic/Networking, so unrelated to this PR.

@CarnaViire
Copy link
Member Author

Test failures are unrelated, so merging

@CarnaViire CarnaViire merged commit 885e7a2 into dotnet:main Aug 17, 2021
@CarnaViire CarnaViire deleted the quic-stream-add-readscompleted branch August 17, 2021 12:34
@karelz karelz added this to the 6.0.0 milestone Aug 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QUIC: Read data and get end notification together
5 participants