From d86be87daa42e73d665cc2b31cedd37ddf52aef5 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 25 Mar 2021 20:31:10 -0400 Subject: [PATCH] Enable CA2012 (Use ValueTask Correctly) (#31221) --- .editorconfig | 6 +++ .../Components/src/Routing/Router.cs | 2 +- .../src/Circuits/RemoteNavigationManager.cs | 2 +- src/Hosting/Hosting/src/Internal/WebHost.cs | 2 +- .../src/Features/RequestServicesFeature.cs | 2 +- .../src/Internal/Http/Http1OutputProducer.cs | 37 ++++++++----------- .../Core/src/Internal/Http/HttpProtocol.cs | 12 +++++- .../FlowControl/StreamInputFlowControl.cs | 4 +- .../src/Internal/Http2/Http2Connection.cs | 6 +-- .../src/Internal/Http2/Http2OutputProducer.cs | 4 +- .../Core/src/Internal/Http2/Http2Stream.cs | 4 +- .../src/Internal/Http3/Http3Connection.cs | 10 ++--- .../src/Internal/Http3/Http3OutputProducer.cs | 2 +- .../src/Internal/QuicConnectionContext.cs | 15 ++------ .../ServerInfrastructure/DuplexPipeStream.cs | 7 ++-- .../samples/ClientSample/Tcp/TcpConnection.cs | 9 +---- .../server/Core/src/HubConnectionContext.cs | 2 +- 17 files changed, 61 insertions(+), 65 deletions(-) diff --git a/.editorconfig b/.editorconfig index 9837244271bf..44bc0ef547c5 100644 --- a/.editorconfig +++ b/.editorconfig @@ -72,3 +72,9 @@ charset = utf-8-bom [*.{cs,vb}] # CA1305: Specify IFormatProvider dotnet_diagnostic.CA1305.severity = error + +[*.{cs,vb}] +# CA2012: Use ValueTask correctly +dotnet_diagnostic.CA2012.severity = warning +[**/{test,perf}/**.{cs,vb}] +dotnet_diagnostic.CA2012.severity = suggestion diff --git a/src/Components/Components/src/Routing/Router.cs b/src/Components/Components/src/Routing/Router.cs index ecb05861f2c3..d2dc133ff1ea 100644 --- a/src/Components/Components/src/Routing/Router.cs +++ b/src/Components/Components/src/Routing/Router.cs @@ -267,7 +267,7 @@ private void OnLocationChanged(object sender, LocationChangedEventArgs args) _locationAbsolute = args.Location; if (_renderHandle.IsInitialized && Routes != null) { - _ = RunOnNavigateAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), args.IsNavigationIntercepted); + _ = RunOnNavigateAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), args.IsNavigationIntercepted).Preserve(); } } diff --git a/src/Components/Server/src/Circuits/RemoteNavigationManager.cs b/src/Components/Server/src/Circuits/RemoteNavigationManager.cs index b57130c29a30..fc297563ab90 100644 --- a/src/Components/Server/src/Circuits/RemoteNavigationManager.cs +++ b/src/Components/Server/src/Circuits/RemoteNavigationManager.cs @@ -75,7 +75,7 @@ protected override void NavigateToCore(string uri, bool forceLoad) throw new NavigationException(absoluteUriString); } - _jsRuntime.InvokeAsync(Interop.NavigateTo, uri, forceLoad); + _jsRuntime.InvokeAsync(Interop.NavigateTo, uri, forceLoad).Preserve(); } private static class Log diff --git a/src/Hosting/Hosting/src/Internal/WebHost.cs b/src/Hosting/Hosting/src/Internal/WebHost.cs index 50c0bbd16903..e20b071d6cd0 100644 --- a/src/Hosting/Hosting/src/Internal/WebHost.cs +++ b/src/Hosting/Hosting/src/Internal/WebHost.cs @@ -334,7 +334,7 @@ public async Task StopAsync(CancellationToken cancellationToken = default) public void Dispose() { - DisposeAsync().GetAwaiter().GetResult(); + DisposeAsync().AsTask().GetAwaiter().GetResult(); } public async ValueTask DisposeAsync() diff --git a/src/Http/Http/src/Features/RequestServicesFeature.cs b/src/Http/Http/src/Features/RequestServicesFeature.cs index e5a80b3450de..eb9bc3b1c421 100644 --- a/src/Http/Http/src/Features/RequestServicesFeature.cs +++ b/src/Http/Http/src/Features/RequestServicesFeature.cs @@ -87,7 +87,7 @@ static async ValueTask Awaited(RequestServicesFeature servicesFeature, ValueTask /// public void Dispose() { - DisposeAsync().GetAwaiter().GetResult(); + DisposeAsync().AsTask().GetAwaiter().GetResult(); } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index 94e63668bee5..ae7059919492 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -58,9 +58,7 @@ internal class Http1OutputProducer : IHttpOutputProducer, IDisposable private bool _autoChunk; - // We rely on the TimingPipeFlusher to give us ValueTasks that can be safely awaited multiple times. private bool _writeStreamSuffixCalled; - private ValueTask _writeStreamSuffixValueTask; private int _advancedBytesForChunk; private Memory _currentChunkMemory; @@ -118,31 +116,27 @@ public ValueTask WriteDataToPipeAsync(ReadOnlySpan buffer, Ca public ValueTask WriteStreamSuffixAsync() { + ValueTask result = default; + lock (_contextLock) { - if (_writeStreamSuffixCalled) + if (!_writeStreamSuffixCalled) { - // If WriteStreamSuffixAsync has already been called, no-op and return the previously returned ValueTask. - return _writeStreamSuffixValueTask; - } + if (_autoChunk) + { + var writer = new BufferWriter(_pipeWriter); + result = WriteAsyncInternal(ref writer, EndChunkedResponseBytes); + } + else if (_unflushedBytes > 0) + { + result = FlushAsync(); + } - if (_autoChunk) - { - var writer = new BufferWriter(_pipeWriter); - _writeStreamSuffixValueTask = WriteAsyncInternal(ref writer, EndChunkedResponseBytes); - } - else if (_unflushedBytes > 0) - { - _writeStreamSuffixValueTask = FlushAsync(); - } - else - { - _writeStreamSuffixValueTask = default; + _writeStreamSuffixCalled = true; } - - _writeStreamSuffixCalled = true; - return _writeStreamSuffixValueTask; } + + return result; } public ValueTask FlushAsync(CancellationToken cancellationToken = default) @@ -533,7 +527,6 @@ public void Reset() _currentMemoryPrefixBytes = 0; _autoChunk = false; _writeStreamSuffixCalled = false; - _writeStreamSuffixValueTask = default; _currentChunkMemoryUpdated = false; _startCalled = false; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index b2816c48257d..bf856a53c4c8 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -917,7 +917,15 @@ public void ProduceContinue() ((IHeaderDictionary)HttpRequestHeaders).TryGetValue(HeaderNames.Expect, out var expect) && (expect.FirstOrDefault() ?? "").Equals("100-continue", StringComparison.OrdinalIgnoreCase)) { - Output.Write100ContinueAsync().GetAwaiter().GetResult(); + ValueTask vt = Output.Write100ContinueAsync(); + if (vt.IsCompleted) + { + vt.GetAwaiter().GetResult(); + } + else + { + vt.AsTask().GetAwaiter().GetResult(); + } } } @@ -1050,6 +1058,8 @@ private Task WriteSuffix() return WriteSuffixAwaited(writeTask); } + writeTask.GetAwaiter().GetResult(); + _requestProcessingStatus = RequestProcessingStatus.ResponseCompleted; if (_keepAlive) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/StreamInputFlowControl.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/StreamInputFlowControl.cs index 7e7c8434e8b4..92a9b9fa588e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/StreamInputFlowControl.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/StreamInputFlowControl.cs @@ -57,7 +57,7 @@ public void UpdateWindows(int bytes) if (streamWindowUpdateSize > 0) { // Writing with the FrameWriter should only fail if given a canceled token, so just fire and forget. - _ = _frameWriter.WriteWindowUpdateAsync(StreamId, streamWindowUpdateSize); + _ = _frameWriter.WriteWindowUpdateAsync(StreamId, streamWindowUpdateSize).Preserve(); } UpdateConnectionWindow(bytes); @@ -90,7 +90,7 @@ private void UpdateConnectionWindow(int bytes) if (connectionWindowUpdateSize > 0) { // Writing with the FrameWriter should only fail if given a canceled token, so just fire and forget. - _ = _frameWriter.WriteWindowUpdateAsync(0, connectionWindowUpdateSize); + _ = _frameWriter.WriteWindowUpdateAsync(0, connectionWindowUpdateSize).Preserve(); } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index 3d1a8628c5a5..6b48daff78d3 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -157,7 +157,7 @@ public void Abort(ConnectionAbortedException ex) { if (TryClose()) { - _frameWriter.WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.INTERNAL_ERROR); + _frameWriter.WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.INTERNAL_ERROR).Preserve(); } _frameWriter.Abort(ex); @@ -1214,7 +1214,7 @@ private void UpdateConnectionState() if (_gracefulCloseInitiator == GracefulCloseInitiator.Server && _clientActiveStreamCount > 0) { - _frameWriter.WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.NO_ERROR); + _frameWriter.WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.NO_ERROR).Preserve(); } } @@ -1224,7 +1224,7 @@ private void UpdateConnectionState() { if (TryClose()) { - _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, Http2ErrorCode.NO_ERROR); + _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, Http2ErrorCode.NO_ERROR).Preserve(); } } else diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 5a6d5633ffe8..01e2b3d69265 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -43,7 +43,7 @@ internal class Http2OutputProducer : IHttpOutputProducer, IHttpOutputAborter, IV private bool _writerComplete; // Internal for testing - internal ValueTask _dataWriteProcessingTask; + internal Task _dataWriteProcessingTask; internal bool _disposed; /// The core logic for the IValueTaskSource implementation. @@ -394,7 +394,7 @@ public void Reset() { } - private async ValueTask ProcessDataWrites() + private async Task ProcessDataWrites() { // ProcessDataWrites runs for the lifetime of the Http2OutputProducer, and is designed to be reused by multiple streams. // When Http2OutputProducer is no longer used (e.g. a stream is aborted and will no longer be used, or the connection is closed) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index ffe6ab76e98d..c8507f9e4bb7 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -145,7 +145,7 @@ public void CompleteStream(bool errored) if (!errored) { // Don't block on IO. This never faults. - _ = _http2Output.WriteRstStreamAsync(Http2ErrorCode.NO_ERROR); + _ = _http2Output.WriteRstStreamAsync(Http2ErrorCode.NO_ERROR).Preserve(); } RequestBodyPipe.Writer.Complete(); } @@ -545,7 +545,7 @@ internal void ResetAndAbort(ConnectionAbortedException abortReason, Http2ErrorCo DecrementActiveClientStreamCount(); // Don't block on IO. This never faults. - _ = _http2Output.WriteRstStreamAsync(error); + _ = _http2Output.WriteRstStreamAsync(error).Preserve(); AbortCore(abortReason); } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 343637bd41e1..df6b535827f4 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -180,7 +180,7 @@ public void Abort(ConnectionAbortedException ex, Http3ErrorCode errorCode) { if (TryClose()) { - SendGoAway(_highestOpenedStreamId); + SendGoAway(_highestOpenedStreamId).Preserve(); } _errorCodeFeature.Error = (long)errorCode; @@ -213,10 +213,10 @@ public void OnTimeout(TimeoutReason reason) switch (reason) { case TimeoutReason.KeepAlive: - SendGoAway(_highestOpenedStreamId); + SendGoAway(_highestOpenedStreamId).Preserve(); break; case TimeoutReason.TimeoutFeature: - SendGoAway(_highestOpenedStreamId); + SendGoAway(_highestOpenedStreamId).Preserve(); break; case TimeoutReason.RequestHeaders: case TimeoutReason.ReadDataRate: @@ -396,7 +396,7 @@ private void UpdateConnectionState() if (_gracefulCloseInitiator == GracefulCloseInitiator.Server && activeRequestCount > 0) { // Go away with largest streamid to initiate graceful shutdown. - SendGoAway(VariableLengthIntegerHelper.EightByteLimit); + SendGoAway(VariableLengthIntegerHelper.EightByteLimit).Preserve(); } } @@ -406,7 +406,7 @@ private void UpdateConnectionState() { if (TryClose()) { - SendGoAway(_highestOpenedStreamId); + SendGoAway(_highestOpenedStreamId).Preserve(); } } else diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index df10c412a487..8e89f84b67aa 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -51,7 +51,7 @@ public Http3OutputProducer( _pipeReader = pipe.Reader; _flusher = new TimingPipeFlusher(_pipeWriter, timeoutControl: null, log); - _dataWriteProcessingTask = ProcessDataWrites(); + _dataWriteProcessingTask = ProcessDataWrites().Preserve(); } public void Dispose() diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs index f3c7da6185d0..4c73ad4084be 100644 --- a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs +++ b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs @@ -19,7 +19,7 @@ internal class QuicConnectionContext : TransportMultiplexedConnection, IProtocol private readonly IQuicTrace _log; private readonly CancellationTokenSource _connectionClosedTokenSource = new CancellationTokenSource(); - private ValueTask _closeTask; + private Task? _closeTask; public long Error { get; set; } @@ -55,15 +55,8 @@ public override async ValueTask DisposeAsync() { try { - if (_closeTask != default) - { - _closeTask = _connection.CloseAsync(errorCode: 0); - await _closeTask; - } - else - { - await _closeTask; - } + _closeTask ??= _connection.CloseAsync(errorCode: 0).AsTask(); + await _closeTask; } catch (Exception ex) { @@ -78,7 +71,7 @@ public override async ValueTask DisposeAsync() public override void Abort(ConnectionAbortedException abortReason) { // dedup calls to abort here. - _closeTask = _connection.CloseAsync(errorCode: Error); + _closeTask = _connection.CloseAsync(errorCode: Error).AsTask(); } public override async ValueTask AcceptAsync(CancellationToken cancellationToken = default) diff --git a/src/Shared/ServerInfrastructure/DuplexPipeStream.cs b/src/Shared/ServerInfrastructure/DuplexPipeStream.cs index 4da736e0f328..9f0e58ee8d98 100644 --- a/src/Shared/ServerInfrastructure/DuplexPipeStream.cs +++ b/src/Shared/ServerInfrastructure/DuplexPipeStream.cs @@ -71,9 +71,10 @@ public override void SetLength(long value) public override int Read(byte[] buffer, int offset, int count) { - // ValueTask uses .GetAwaiter().GetResult() if necessary - // https://github.com/dotnet/corefx/blob/f9da3b4af08214764a51b2331f3595ffaf162abe/src/System.Threading.Tasks.Extensions/src/System/Threading/Tasks/ValueTask.cs#L156 - return ReadAsyncInternal(new Memory(buffer, offset, count), default).Result; + ValueTask vt = ReadAsyncInternal(new Memory(buffer, offset, count), default); + return vt.IsCompleted ? + vt.Result : + vt.AsTask().GetAwaiter().GetResult(); } public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken = default) diff --git a/src/SignalR/samples/ClientSample/Tcp/TcpConnection.cs b/src/SignalR/samples/ClientSample/Tcp/TcpConnection.cs index d64b5b8fb951..8843ab4941c0 100644 --- a/src/SignalR/samples/ClientSample/Tcp/TcpConnection.cs +++ b/src/SignalR/samples/ClientSample/Tcp/TcpConnection.cs @@ -168,14 +168,7 @@ private async Task ProcessReceives() _application.Output.Advance(bytesReceived); - var flushTask = _application.Output.FlushAsync(); - - if (!flushTask.IsCompleted) - { - await flushTask; - } - - var result = flushTask.GetAwaiter().GetResult(); + var result = await _application.Output.FlushAsync(); if (result.IsCompleted) { // Pipe consumer is shut down, do we stop writing diff --git a/src/SignalR/server/Core/src/HubConnectionContext.cs b/src/SignalR/server/Core/src/HubConnectionContext.cs index b2de9b6e16ca..c7a399264083 100644 --- a/src/SignalR/server/Core/src/HubConnectionContext.cs +++ b/src/SignalR/server/Core/src/HubConnectionContext.cs @@ -625,7 +625,7 @@ private void KeepAliveTick() // If the transport channel is full, this will fail, but that's OK because // adding a Ping message when the transport is full is unnecessary since the // transport is still in the process of sending frames. - _ = TryWritePingAsync(); + _ = TryWritePingAsync().Preserve(); // We only update the timestamp here, because updating on each sent message is bad for performance // There can be a lot of sent messages per 15 seconds