-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[QUIC] QuicStream add ReadsCompleted #57492
Conversation
Note regarding the 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. |
Tagging subscribers to this area: @dotnet/ncl Issue DetailsAdd 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
|
@@ -788,5 +789,80 @@ public async Task BigWrite_SmallRead_Success(bool closeWithData) | |||
} | |||
} | |||
} | |||
|
|||
[Fact] | |||
public async Task BasicTest_WithReadsCompletedCheck() |
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.
Unfinished test name?
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.
The name's like that because it is basically the same test as QuicStreamTests.BasicTest
runtime/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs
Lines 23 to 55 in 42af636
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
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.
Ah. I wasn't familiar with BasicTest so this name seemed non-descript. I'll let you decide what to do with it.
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
Outdated
Show resolved
Hide resolved
Thanks @CarnaViire! |
Browser runs are hitting #57501, unrelated to this PR. |
Test failures are unrelated, so merging |
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