From 3a9df4df7ae0d8886eac9c91fe830e22b05288be Mon Sep 17 00:00:00 2001 From: Maxim Kim Date: Wed, 11 Jan 2023 18:13:07 -0800 Subject: [PATCH 1/6] Use SslStream.WriteAsync instead of SslStream.Write on netcore+ to avoid eventloop blocking --- src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs index 026b350f..076cc645 100644 --- a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs +++ b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs @@ -173,7 +173,13 @@ private void Wrap(IChannelHandlerContext context) _lastContextWritePromise = promise; if (buf.IsReadable()) { +#if NETCOREAPP_2_0_GREATER || NETSTANDARD_2_0_GREATER + var data = buf.GetReadableMemory(); + _ = LinkOutcome(_sslStream.WriteAsync(data, CancellationToken.None), promise); // this leads to FinishWrap being called 0+ times + buf.AdvanceReader(readableBytes); +#else _ = buf.ReadBytes(_sslStream, readableBytes); // this leads to FinishWrap being called 0+ times +#endif } else if (promise != null) { From 55040cc79adf52cf495bb0551e2e022d3a5f2ac1 Mon Sep 17 00:00:00 2001 From: Maxim Kim Date: Thu, 12 Jan 2023 23:33:41 -0800 Subject: [PATCH 2/6] avoid overlapping writes into SslStream --- .../Tls/TlsHandler.MediationStream.NetCore.cs | 8 ++- .../Tls/TlsHandler.MediationStream.NetFx.cs | 2 +- ...lsHandler.MediationStream.NetStandard20.cs | 6 +- .../Tls/TlsHandler.Writer.cs | 63 ++++++++++++++----- 4 files changed, 61 insertions(+), 18 deletions(-) diff --git a/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetCore.cs b/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetCore.cs index ada9d6e6..cd136922 100644 --- a/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetCore.cs +++ b/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetCore.cs @@ -184,11 +184,15 @@ public override void Write(byte[] buffer, int offset, int count) public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) { - return new ValueTask(_owner.FinishWrapNonAppDataAsync(buffer, _owner._lastContextWritePromise ?? _owner.CapturedContext.NewPromise())); + var promise = _owner._asyncWritePromises.TryDequeue(out var queuedPromise) ? queuedPromise : _owner.CapturedContext.NewPromise(); + return new ValueTask(_owner.FinishWrapAsync(buffer, promise)); } public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) - => _owner.FinishWrapNonAppDataAsync(buffer, offset, count, _owner._lastContextWritePromise ?? _owner.CapturedContext.NewPromise()); + { + var promise = _owner._asyncWritePromises.TryDequeue(out var queuedPromise) ? queuedPromise : _owner.CapturedContext.NewPromise(); + return _owner.FinishWrapAsync(buffer, offset, count, promise); + } } } } diff --git a/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetFx.cs b/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetFx.cs index 96f98b51..db83326a 100644 --- a/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetFx.cs +++ b/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetFx.cs @@ -148,7 +148,7 @@ private int ReadFromInput(byte[] destination, int destinationOffset, int destina public override void Write(byte[] buffer, int offset, int count) => _owner.FinishWrap(buffer, offset, count, _owner._lastContextWritePromise ?? _owner.CapturedContext.VoidPromise()); public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) - => _owner.FinishWrapNonAppDataAsync(buffer, offset, count, _owner._lastContextWritePromise ?? _owner.CapturedContext.NewPromise()); + => _owner.FinishWrapAsync(buffer, offset, count, _owner._lastContextWritePromise ?? _owner.CapturedContext.NewPromise()); private static readonly Action s_writeCompleteCallback = (t, s) => HandleChannelWriteComplete(t, s); diff --git a/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetStandard20.cs b/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetStandard20.cs index 688110d7..d219dacc 100644 --- a/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetStandard20.cs +++ b/src/DotNetty.Handlers/Tls/TlsHandler.MediationStream.NetStandard20.cs @@ -111,7 +111,11 @@ private int ReadFromInput(byte[] destination, int destinationOffset, int destina public override void Write(byte[] buffer, int offset, int count) => _owner.FinishWrap(buffer, offset, count, _owner._lastContextWritePromise ?? _owner.CapturedContext.NewPromise()); public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) - => _owner.FinishWrapNonAppDataAsync(buffer, offset, count, _owner._lastContextWritePromise ?? _owner.CapturedContext.NewPromise()); + { + var promiseQueue = _owner._asyncWritePromises; + var promise = promiseQueue.Count > 0 ? promiseQueue.Dequeue() : _owner.CapturedContext.NewPromise(); + return _owner.FinishWrapAsync(buffer, offset, count, promise); + } } } } diff --git a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs index 076cc645..a4246334 100644 --- a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs +++ b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs @@ -23,6 +23,7 @@ namespace DotNetty.Handlers.Tls { using System; + using System.Collections.Generic; using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; @@ -43,6 +44,11 @@ partial class TlsHandler private IPromise _lastContextWritePromise; private volatile int v_wrapDataSize = TlsUtils.MAX_PLAINTEXT_LENGTH; + +#if NETCOREAPP_2_0_GREATER || NETSTANDARD_2_0_GREATER || NETSTANDARD2_0 + private readonly Queue _asyncWritePromises = new Queue(10); + private ValueTask _lastAsyncWrite; +#endif /// /// Gets or Sets the number of bytes to pass to each call. @@ -174,9 +180,12 @@ private void Wrap(IChannelHandlerContext context) if (buf.IsReadable()) { #if NETCOREAPP_2_0_GREATER || NETSTANDARD_2_0_GREATER - var data = buf.GetReadableMemory(); - _ = LinkOutcome(_sslStream.WriteAsync(data, CancellationToken.None), promise); // this leads to FinishWrap being called 0+ times - buf.AdvanceReader(readableBytes); + var writeTask = WriteAsync(buf, promise); + if (!writeTask.IsCompleted) + { + _lastAsyncWrite = writeTask; + } + buf = null; //prevent buf from releasing synchronously #else _ = buf.ReadBytes(_sslStream, readableBytes); // this leads to FinishWrap being called 0+ times #endif @@ -188,15 +197,12 @@ private void Wrap(IChannelHandlerContext context) } catch (Exception exc) { - promise.TrySetException(exc); - // SslStream has been closed already. - // Any further write attempts should be denied. - _pendingUnencryptedWrites?.ReleaseAndFailAll(exc); + OnWriteFailure(exc, promise); throw; } finally { - buf.Release(); + buf?.Release(); buf = null; promise = null; _lastContextWritePromise = null; @@ -249,7 +255,7 @@ private void FinishWrap(byte[] buffer, int offset, int count, IPromise promise) } #if NETCOREAPP || NETSTANDARD_2_0_GREATER - private Task FinishWrapNonAppDataAsync(in ReadOnlyMemory buffer, IPromise promise) + private Task FinishWrapAsync(in ReadOnlyMemory buffer, IPromise promise) { var capturedContext = CapturedContext; Task future; @@ -266,7 +272,7 @@ private Task FinishWrapNonAppDataAsync(in ReadOnlyMemory buffer, IPromise } #endif - private Task FinishWrapNonAppDataAsync(byte[] buffer, int offset, int count, IPromise promise) + private Task FinishWrapAsync(byte[] buffer, int offset, int count, IPromise promise) { var capturedContext = CapturedContext; var future = capturedContext.WriteAndFlushAsync(Unpooled.WrappedBuffer(buffer, offset, count), promise); @@ -275,20 +281,49 @@ private Task FinishWrapNonAppDataAsync(byte[] buffer, int offset, int count, IPr } #if NETCOREAPP || NETSTANDARD_2_0_GREATER - private static async ValueTask LinkOutcome(ValueTask valueTask, IPromise promise) + private async ValueTask WriteAsync(IByteBuffer buf, IPromise promise) { + if (!_lastAsyncWrite.IsCompletedSuccessfully) + { + try + { + await _lastAsyncWrite; + } + catch (Exception ex) + { + buf.AdvanceReader(buf.ReadableBytes); + buf.Release(); + promise.TrySetException(ex); + return; + } + } + + var mem = buf.GetReadableMemory(); try { - await valueTask; - promise.TryComplete(); + _asyncWritePromises.Enqueue(promise); + await _sslStream.WriteAsync(mem, CancellationToken.None); // this leads to FinishWrapAsync being called 0+ times } catch (Exception ex) { - promise.TrySetException(ex); + OnWriteFailure(ex, promise); + } + finally + { + buf.AdvanceReader(mem.Length); + buf.Release(); } } #endif + private void OnWriteFailure(Exception ex, IPromise promise) + { + promise.TrySetException(ex); + // SslStream has been closed already. + // Any further write attempts should be denied. + _pendingUnencryptedWrites?.ReleaseAndFailAll(ex); + } + [MethodImpl(MethodImplOptions.NoInlining)] private static InvalidOperationException NewPendingWritesNullException() { From 45f89abf38ab26456698c5a36d706bfcab16fc97 Mon Sep 17 00:00:00 2001 From: Maxim Kim Date: Fri, 13 Jan 2023 23:29:51 -0800 Subject: [PATCH 3/6] propagate write failure to the next write --- .../Tls/TlsHandler.Writer.cs | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs index a4246334..8b816caf 100644 --- a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs +++ b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs @@ -47,7 +47,7 @@ partial class TlsHandler #if NETCOREAPP_2_0_GREATER || NETSTANDARD_2_0_GREATER || NETSTANDARD2_0 private readonly Queue _asyncWritePromises = new Queue(10); - private ValueTask _lastAsyncWrite; + private Task _lastAsyncWrite; #endif /// @@ -180,10 +180,12 @@ private void Wrap(IChannelHandlerContext context) if (buf.IsReadable()) { #if NETCOREAPP_2_0_GREATER || NETSTANDARD_2_0_GREATER - var writeTask = WriteAsync(buf, promise); - if (!writeTask.IsCompleted) + var asyncWrite = WriteAsync(buf, promise); + if (!asyncWrite.IsCompleted) { - _lastAsyncWrite = writeTask; + var asyncWriteTask = asyncWrite.AsTask(); + asyncWriteTask.Ignore(); + _lastAsyncWrite = asyncWriteTask; } buf = null; //prevent buf from releasing synchronously #else @@ -283,34 +285,37 @@ private Task FinishWrapAsync(byte[] buffer, int offset, int count, IPromise prom #if NETCOREAPP || NETSTANDARD_2_0_GREATER private async ValueTask WriteAsync(IByteBuffer buf, IPromise promise) { - if (!_lastAsyncWrite.IsCompletedSuccessfully) + var lastAsyncWrite = _lastAsyncWrite; + if (lastAsyncWrite != null && !_lastAsyncWrite.IsCompletedSuccessfully) { try { - await _lastAsyncWrite; + await lastAsyncWrite; } catch (Exception ex) { - buf.AdvanceReader(buf.ReadableBytes); + //handle failure and propagate to the next pending write buf.Release(); promise.TrySetException(ex); - return; + throw; } } - var mem = buf.GetReadableMemory(); try { _asyncWritePromises.Enqueue(promise); + var mem = buf.GetReadableMemory(); await _sslStream.WriteAsync(mem, CancellationToken.None); // this leads to FinishWrapAsync being called 0+ times + buf.AdvanceReader(mem.Length); } catch (Exception ex) { + //handle failure and propagate to the next pending write OnWriteFailure(ex, promise); + throw; } finally { - buf.AdvanceReader(mem.Length); buf.Release(); } } From 72575834eeb142c18cdf10b353f3635fc363eceb Mon Sep 17 00:00:00 2001 From: Maxim Kim Date: Fri, 13 Jan 2023 23:44:34 -0800 Subject: [PATCH 4/6] fix field access --- src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs index 8b816caf..3f09fbd6 100644 --- a/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs +++ b/src/DotNetty.Handlers/Tls/TlsHandler.Writer.cs @@ -286,7 +286,7 @@ private Task FinishWrapAsync(byte[] buffer, int offset, int count, IPromise prom private async ValueTask WriteAsync(IByteBuffer buf, IPromise promise) { var lastAsyncWrite = _lastAsyncWrite; - if (lastAsyncWrite != null && !_lastAsyncWrite.IsCompletedSuccessfully) + if (lastAsyncWrite != null && !lastAsyncWrite.IsCompletedSuccessfully) { try { From b937ca650688694ab83d8f4a6b4d4622cf5d83eb Mon Sep 17 00:00:00 2001 From: nimakamoosi <105183099+nimakamoosi@users.noreply.github.com> Date: Wed, 28 Feb 2024 10:00:36 -0500 Subject: [PATCH 5/6] Disable incremental builds and tests --- build/pr-netfx-validation.yaml | 4 ++-- build/pr-validation.yaml | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/build/pr-netfx-validation.yaml b/build/pr-netfx-validation.yaml index e3bf32c6..56da4126 100644 --- a/build/pr-netfx-validation.yaml +++ b/build/pr-netfx-validation.yaml @@ -42,7 +42,7 @@ stages: displayName: Windows Build inputs: filename: build.cmd - arguments: 'Build incremental' # Run an incremental build + arguments: 'Build' #Add ' incremental' to build incremental-ly continueOnError: true condition: eq( variables['Agent.OS'], 'Windows_NT' ) - task: CopyFiles@2 @@ -63,7 +63,7 @@ stages: # displayName: 'Unit Tests (Windows 2019)' # vmImage: 'windows-2019' # scriptFileName: build.cmd - # scriptArgs: RunTestsNetFx471 incremental + # scriptArgs: RunTestsNetFx471 #Add ' incremental' to Run tests incremental-ly # outputDirectory: 'TestResults' # artifactName: 'netfx_tests_windows-$(Build.BuildId)' # packageFeed: $(packageFeed) \ No newline at end of file diff --git a/build/pr-validation.yaml b/build/pr-validation.yaml index 607b288f..ee20408d 100644 --- a/build/pr-validation.yaml +++ b/build/pr-validation.yaml @@ -42,7 +42,7 @@ stages: displayName: Windows Build inputs: filename: build.cmd - arguments: 'Build incremental' # Run an incremental build + arguments: 'Build' #Add ' incremental' to build incremental-ly continueOnError: true condition: eq( variables['Agent.OS'], 'Windows_NT' ) - task: CopyFiles@2 @@ -63,8 +63,8 @@ stages: displayName: 'Unit Tests (Windows 2022)' vmImage: 'windows-2022' scriptFileName: build.cmd - scriptArgs: RunTests incremental - outputDirectory: 'TestResults' + scriptArgs: RunTests + outputDirectory: 'TestResults' #Add ' incremental' to Run tests incremental-ly artifactName: 'net_core_tests_windows-$(Build.BuildId)' packageFeed: $(packageFeed) @@ -74,7 +74,7 @@ stages: displayName: 'Unit Tests (Windows 2019)' vmImage: 'windows-2019' scriptFileName: build.cmd - scriptArgs: RunTests incremental + scriptArgs: RunTests #Add ' incremental' to Run tests incremental-ly outputDirectory: 'TestResults' artifactName: 'net_core_tests_windows-$(Build.BuildId)' packageFeed: $(packageFeed) @@ -89,7 +89,7 @@ stages: displayName: 'Unit Tests (Ubuntu-20)' vmImage: 'ubuntu-20.04' scriptFileName: './build.sh' - scriptArgs: RunTests incremental + scriptArgs: RunTests #Add ' incremental' to Run tests incremental-ly outputDirectory: 'TestResults' artifactName: 'net_core_tests_ubuntu_16-$(Build.BuildId)' packageFeed: $(packageFeed) @@ -100,7 +100,7 @@ stages: displayName: 'Unit Tests (Ubuntu-22)' vmImage: 'ubuntu-22.04' scriptFileName: './build.sh' - scriptArgs: RunTests incremental + scriptArgs: RunTests #' incremental' # Run tests incremental-ly outputDirectory: 'TestResults' artifactName: 'net_core_tests_ubuntu_22-$(Build.BuildId)' packageFeed: $(packageFeed) \ No newline at end of file From 0b0070e932b84c6aa29ed888eabd05b4c77edaac Mon Sep 17 00:00:00 2001 From: nimakamoosi <105183099+nimakamoosi@users.noreply.github.com> Date: Mon, 11 Mar 2024 20:12:19 -0400 Subject: [PATCH 6/6] Minor comment changes. --- build/pr-netfx-validation.yaml | 4 ++-- build/pr-validation.yaml | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/build/pr-netfx-validation.yaml b/build/pr-netfx-validation.yaml index 56da4126..de649b0a 100644 --- a/build/pr-netfx-validation.yaml +++ b/build/pr-netfx-validation.yaml @@ -42,7 +42,7 @@ stages: displayName: Windows Build inputs: filename: build.cmd - arguments: 'Build' #Add ' incremental' to build incremental-ly + arguments: 'Build' continueOnError: true condition: eq( variables['Agent.OS'], 'Windows_NT' ) - task: CopyFiles@2 @@ -63,7 +63,7 @@ stages: # displayName: 'Unit Tests (Windows 2019)' # vmImage: 'windows-2019' # scriptFileName: build.cmd - # scriptArgs: RunTestsNetFx471 #Add ' incremental' to Run tests incremental-ly + # scriptArgs: RunTestsNetFx471 # outputDirectory: 'TestResults' # artifactName: 'netfx_tests_windows-$(Build.BuildId)' # packageFeed: $(packageFeed) \ No newline at end of file diff --git a/build/pr-validation.yaml b/build/pr-validation.yaml index ee20408d..05396fab 100644 --- a/build/pr-validation.yaml +++ b/build/pr-validation.yaml @@ -42,7 +42,7 @@ stages: displayName: Windows Build inputs: filename: build.cmd - arguments: 'Build' #Add ' incremental' to build incremental-ly + arguments: 'Build' continueOnError: true condition: eq( variables['Agent.OS'], 'Windows_NT' ) - task: CopyFiles@2 @@ -64,7 +64,7 @@ stages: vmImage: 'windows-2022' scriptFileName: build.cmd scriptArgs: RunTests - outputDirectory: 'TestResults' #Add ' incremental' to Run tests incremental-ly + outputDirectory: 'TestResults' artifactName: 'net_core_tests_windows-$(Build.BuildId)' packageFeed: $(packageFeed) @@ -74,7 +74,7 @@ stages: displayName: 'Unit Tests (Windows 2019)' vmImage: 'windows-2019' scriptFileName: build.cmd - scriptArgs: RunTests #Add ' incremental' to Run tests incremental-ly + scriptArgs: RunTests outputDirectory: 'TestResults' artifactName: 'net_core_tests_windows-$(Build.BuildId)' packageFeed: $(packageFeed) @@ -89,7 +89,7 @@ stages: displayName: 'Unit Tests (Ubuntu-20)' vmImage: 'ubuntu-20.04' scriptFileName: './build.sh' - scriptArgs: RunTests #Add ' incremental' to Run tests incremental-ly + scriptArgs: RunTests outputDirectory: 'TestResults' artifactName: 'net_core_tests_ubuntu_16-$(Build.BuildId)' packageFeed: $(packageFeed) @@ -100,7 +100,7 @@ stages: displayName: 'Unit Tests (Ubuntu-22)' vmImage: 'ubuntu-22.04' scriptFileName: './build.sh' - scriptArgs: RunTests #' incremental' # Run tests incremental-ly + scriptArgs: RunTests outputDirectory: 'TestResults' artifactName: 'net_core_tests_ubuntu_22-$(Build.BuildId)' packageFeed: $(packageFeed) \ No newline at end of file