[QUIC] QuicStream reading/writing work
#90253
Conversation
…hutdown in dispose, named constants
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsSo far address:
Remain as-is: Still working on: Fixes #77216, #79911, #82704 Also fixed problem with GC eating Affected tests were locally ran with iteration count 100 before committing the fix. Putting up as draft to get early feedback and CI results.
|
|
So far: outerloop unrelated failures, stress H/3 clear. |
|
|
||
| unsafe | ||
| { | ||
| _sendBuffers.Initialize(buffer); | ||
| int status = MsQuicApi.Api.StreamSend( |
There was a problem hiding this comment.
Take into account that abort might have happened in parallel or in the meantime and StreamSend might return an error.
There was a problem hiding this comment.
It'll return QUIC_STATUS_ABORTED which is conveniently ignored by TryGetStreamExceptionForMsQuicStatus, leading to a correct path of throwing exception from Abort and overriding it with OperationCanceled in finally block.
…with FIN, we will do that.
ManickaP
force-pushed
the
mapichov/quic-stream
branch
from
6d1b71f to
978abf7
Compare
| /// It remembers the result from <see cref="TryComplete"/> and propagates it to <see cref="_finalTaskSource"/> only after <see cref="TrySignal"/> is called. | ||
| /// Effectively allowing to separate setting of the result from task completion, which is necessary when the resettable portion of the value task source needs to consumed first. | ||
| /// </summary> | ||
| private struct FinalTaskSource |
There was a problem hiding this comment.
This is the original implementation for FinalTaskSource, I only brought it back.
It solves the problem when Writes/ReadsClosed tasks were signaled long before ValueTask. Now, they are always signaled afterwards.
| _valueTaskSource.SetException(_finalTaskSource.Task.Exception?.InnerException!); | ||
| } | ||
|
|
||
| // In case the _valueTaskSource was successful, we want the potential error from _finalTaskSource to surface immediately. |
There was a problem hiding this comment.
Removed this part as it was causing #82523
| } | ||
| // The task was non-finally completed without having anyone awaiting on it. | ||
| // In such case, discard the temporary result and replace it with this final completion. | ||
| if (state == State.Ready && !_hasWaiter && final) |
There was a problem hiding this comment.
This allows to ignore RECEIVE event notification that no ReadAsync is waiting on, allowing AbortRead to override them.
|
|
||
| private bool TryComplete(Exception? exception, bool final) | ||
| { | ||
| // Dispose the cancellation registration before completing the task, so that it cannot run after the awaiting method returned. |
There was a problem hiding this comment.
Solves #82704.
As the registration was disposed after completing the tasks, they continuation was allowed to run, which reset this value task source while cancellation registration was still alive and could fire before the disposal.
| { | ||
| thisRef._cancellationAction?.Invoke(target); | ||
| thisRef._cancelledToken = cancellationToken; |
There was a problem hiding this comment.
We don't set the task from cancellation, now it's part of cancellation action. But we remember that it was cancelled via this token and throw appropriate error in GetResult, effectively overriding any result set to the task.
This helps with waiting on SEND_COMPLETE in WriteAsync where we don't want cancellation to unblock the task, but we still want to propagate it to the user when SEND_COMPLETE arrives.
| _finalTaskSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); | ||
| if (!TrySignal(out _)) | ||
| { | ||
| GCHandle handle = GCHandle.Alloc(keepAlive); |
There was a problem hiding this comment.
Solves GC eating the Stream when the only thing awaited was Reads/WritesClosed.
| @@ -109,7 +110,8 @@ public sealed partial class QuicStream | |||
| } | |||
| }; | |||
| private MsQuicBuffers _sendBuffers = new MsQuicBuffers(); | |||
| private readonly object _sendBuffersLock = new object(); | |||
| private int _sendLocked; | |||
There was a problem hiding this comment.
When taken, the owner has the exclusive right to unblock _sendTcs. Used to make sure that when StreamSend is called and succeeds (WriteAsync takes the lock), we will always wait for SEND_COMPLETE (event handle returns it). If Abort happens in the meantime, it will store the exception in _sendException instead.
| @@ -462,6 +461,21 @@ public void Abort(QuicAbortDirection abortDirection, long errorCode) | |||
| (ulong)errorCode), | |||
| "StreamShutdown failed"); | |||
| } | |||
|
|
|||
| if (abortDirection.HasFlag(QuicAbortDirection.Read)) | |||
There was a problem hiding this comment.
This reordering solves #79911.
ManickaP
force-pushed
the
mapichov/quic-stream
branch
2 times, most recently
from
5c74b9c to
d5f8381
Compare
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -150,6 +151,7 @@ private void CheckForShutdown() | |||
|
|
|||
| if (_clientControl != null) | |||
| { | |||
| await _sendSettingsTask.ConfigureAwait(false); | |||
There was a problem hiding this comment.
Is it possible this task throws? E.g. connection closed before we were able to send settings? Or it is impossible at this point in time?
There was a problem hiding this comment.
No, SendSettingsAsync catches all exceptions and calls Abort on them:
| else if (closeType == CloseOutboundControlStream.Abort) | ||
| { | ||
| int iterations = 5; | ||
| while (iterations-- > 0) |
There was a problem hiding this comment.
why are iterations and delay needed?
There was a problem hiding this comment.
Depending on timing, the client can receive the control stream abort, i.e. RESET_STREAM, before it even read the HTTP/3 stream type from it, see
QUIC spec says, RESET can discard data, which can happen in this case. Add to it H/3 spec, which says that unrecognized or aborted stream should be ignored:
Leading to ignoring the incoming control stream, because client doesn't know it's control, instead of reacting to the abort and closing the connection.
So this fix attempts to do the again, but with slight delay between sending the data and abort, line 1710.
And why this worked before: we would return data for one QuicStream.ReadAsync before propagating the error, not the whole buffer, just one call to read succeed, which was enough to get that 1 byte saying it's control. Also, MsQuic could always discard the data, we just never observed it (timing, too fast on the same machine...)
There was a problem hiding this comment.
While the change seems LGTM, but I see in the latest CI run that there was an assert firing on dispose, might be worth investigating...
The assert in question is a new one added to QuicStream's DisposeAsync (Line 717): Debug.Assert(_sendTcs.IsCompleted);
Process terminated. Assertion failed.
at System.Net.Quic.QuicStream.DisposeAsync() in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs:line 717
at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs:line 38
at System.Net.Quic.QuicStream.DisposeAsync()
at System.Net.Quic.QuicStream.Dispose(Boolean disposing) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.Stream.cs:line 222
at System.IO.Stream.Close() in /_/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs:line 165
at System.Net.Http.Http3RequestStream.Dispose() in /_/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs:line 93
at System.Net.Http.Http3RequestStream.Http3ReadStream.Dispose(Boolean disposing) in /_/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs:line 1395
at System.IO.Stream.Close() in /_/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs:line 165
at System.Net.Http.HttpConnectionResponseContent.Dispose(Boolean disposing) in /_/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionResponseContent.cs:line 89
at System.Net.Http.HttpContent.Dispose() in /_/src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs:line 679
at System.Net.Http.HttpResponseMessage.Dispose(Boolean disposing) in /_/src/libraries/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs:line 215
at System.Net.Http.HttpResponseMessage.Dispose() in /_/src/libraries/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs:line 221
at System.Net.Http.Functional.Tests.HttpClientHandlerTest_Http3.<>c__DisplayClass14_0.<<RequestSendingResponseDisposed_ThrowsOnServer>b__1>d.MoveNext() in /_/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs:line 573
...
|
Failure is: #91308 |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
So far addressed:
QuicStream.WritesClosedcontinuation ran before the Quic reset frame is sent? #79911 - tested on repro (5+ mins), fixedQuicStream.WriteAsynccancellation token is canceled afterWriteAsynccompletes #82704 - tested on repro (5+ mins), fixedQuicStream.WriteAsyncexception #82523 - tested on repro (5+ mins), fixedRemain as-is:
QuicStream.ReadsClosedcompletes before EOS is consumed #79818QuicStream.ReadsCloseddoesn't always complete on connection disposal #77216Closes #77216
Fixes #79911
Fixes #82704
Closes #79818
Fixes #82523
Also fixed problem with GC eating
QuicStreamwhen only eitherReadsClosedorWritesClosedis awaited. Improved some logging. Added bunch of tests.Affected tests were locally ran with iteration count 100, repro cases ran for 5+ minutes, ran ASP.NET Core tests locally and running stress test in docker locally now (result will be later).
cc @CarnaViire @rzikm @wfurt