Do abortive connection shutdown in MsQuicConnection.Dispose #55493
Conversation
… for CloseAsync and Dispose, and remove CloseAsync from RunClientServer as it should not be necessary anymore
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #55492 Also add tests for QuicConnection.CloseAsync and QuicConnection.Dispose, and update the RunClientServer logic.
|
| } | ||
|
|
||
| [Theory] | ||
| // [InlineData(0)] // Fails with TimeoutException -- needs investigation |
There was a problem hiding this comment.
Isn't it because since you don't do any write, the stream start won't kick off and nothing gets actually sent over the wire?
There was a problem hiding this comment.
It's probably related to that. But it still seems like stream should get failed, even if it hasn't sent anything on the wire yet.
There was a problem hiding this comment.
This is indeed because the stream won't be created on the wire until you write at least 1 byte to it, so serverConnection.AcceptStreamAsync() never finishes - and so semaphore is never released, and so connection is not closed, and so there's no failure, just a hang.
Funny thing is if you call CloseAsync without waiting for semaphore, this would actually flush stream creation event and peer's AcceptStreamAsync would finish.
I wonder if there's a way to manually flush connection events. If not, I don't think there's much we can do.
There was a problem hiding this comment.
Yeah, you are right of course -- the timeout comes from the AcceptStreamAsync.
I wonder if there's a way to manually flush connection events. If not, I don't think there's much we can do.
I think that FlushAsync on a stream that hasn't been started on the wire should force it to be started on the wire.
That would simplify a few things, like the stream construction logic we use for the Stream Conformance Tests, where we currently have to send and receive a byte in order to force stream creation before using it for the tests.
But that's not something we need to tackle right now, I think. Besides, it gets in to the broader discussion about send buffering and flushing that we are punting for 6.0.
There was a problem hiding this comment.
All that said, for now I am just going to remove these "0" variations, since they end up behaving quite differently than the others. If you think it's worthwhile to have tests for this, let's figure out how to add them later; but they are probably separate tests.
| MsQuicApi.Api.ConnectionShutdownDelegate( | ||
| _state.Handle, | ||
| QUIC_CONNECTION_SHUTDOWN_FLAGS.SILENT, | ||
| 0); |
There was a problem hiding this comment.
It feels like it should be configurable from somewhere, but we might want to at least have it as a constant DEFAULT_ABORT_CODE or something like this.
There was a problem hiding this comment.
This value doesn't actually matter, because of the SILENT flag -- no error is sent to the peer.
In general though, I do agree we should define DEFAULT_ABORT_CODE or something like that.
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
Outdated
Show resolved
Hide resolved
|
Failure are unrelated. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #55492
Also add tests for QuicConnection.CloseAsync and QuicConnection.Dispose, and update the RunClientServer logic.