-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[QUIC] Add ShutdownCompleted method and fix receive and shutdown behavior in tests #50930
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
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsRelated to microsoft/msquic#1384
This PR fixed
Contributes to #49157
|
|
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. |
|
We are still discussing the API design here, right? #756 I'm fine to get this in if it unblocks tests, but we may end up reworking the API here later. |
|
@geoffkizer It's totally fine to rework APIs later, I understand that. My goal is to make tests working correctly as even basic ones didn't work before. Without green tests it will be harder to rework stuff 😄 |
ManickaP
left a comment
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.
Looks good, thanks!
I give it one more go once the questions are solved.
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs
Outdated
Show resolved
Hide resolved
| using QuicStream clientStream = clientConnection.OpenBidirectionalStream(); | ||
| Assert.Equal(0, clientStream.StreamId); | ||
|
|
||
| // TODO: stream that is opened by client but left unaccepted by server may cause AccessViolationException in its Finalizer |
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.
AccessViolationException is always wrong (memory corruption), we should not be causing that in any case.
By any chance do you have details of this exception? Dump maybe?
| continue; | ||
| } | ||
|
|
||
| Console.WriteLine($"Wrong data starting from pos={i}"); |
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.
If you need to log in test, you should rather use ITestOutputHelper, console output gets suppressed by xUnit.
| public HttpClientHandlerTestBase(ITestOutputHelper output) |
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.
ITestOutputHelper doesn't output to console if the tests are run from the command line... as the method I've added is more an extended assert than just some test logging, I guess it will be more useful then to use an Assert method that has a message parameter.
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
Outdated
Show resolved
Hide resolved
ManickaP
left a comment
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.
LGTM, thanks.
Related to microsoft/msquic#1384
QuicStreamTests.GetStreamIdWithoutStartWorks)HandleEventShutdownCompleteThis PR fixed
QuicStreamTests.BasicTestandQuicStreamTests.MultipleReadsAndWrites.QuicConnectionTests.AcceptStream_ConnectionAborted_ByClient_Throwsstarted to fail after updating msquic from microsoft/msquic@cc104e8 to microsoft/msquic@8e21db7. I haven't investigated it yet, will add to #49157.QuicStreamTests.LargeDataSentAndReceivedandQuicStreamTests.ReadWrite_Random_Successstill have issues with bytes mixing in the middle of data -- if run together with all the tests. They won't fail if executed alone or all test executed sequentially. The minimal repro for the issue to show should be to combineQuicStreamTests.LargeDataSentAndReceivedandMsQuicTests.CallDifferentWriteMethodsWorksin a single test as two concurrent tasks. I haven't investigated it further yet.Contributes to #49157