From d5cf967a66280e5e2bf7699722930282609e0c05 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Thu, 12 Aug 2021 16:51:29 +0200 Subject: [PATCH 1/4] do not drain HttpContentReadStream if the connection is disposed --- .../Net/Http/SocketsHttpHandler/HttpContentReadStream.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs index 4daad8f2b0ec76..8ae577dfaf844c 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs @@ -46,7 +46,8 @@ protected override void Dispose(bool disposing) return; } - if (disposing && NeedsDrain) + // The connection might be disposed because of cancellation. We should not drain the response in this case. + if (disposing && NeedsDrain && _connection?._disposed != Status_Disposed) { // Start the asynchronous drain. // It may complete synchronously, in which case the connection will be put back in the pool synchronously. From 9e8f5ba5dd39e408bf40f89a7c3d31114d86e358 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Fri, 13 Aug 2021 11:18:03 +0200 Subject: [PATCH 2/4] move _connection checking to NeedsDrain --- .../Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs | 4 +++- .../Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs | 4 +++- .../Net/Http/SocketsHttpHandler/HttpContentReadStream.cs | 3 +-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs index 5d0fb49db747aa..d8c153be3fd4c6 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs @@ -425,7 +425,9 @@ private enum ParsingState : byte Done } - public override bool NeedsDrain => (_connection != null); + // _connection == null typically means that we have finished reading the response. + // Cancellation may lead to a state where a disposed _connection is not null. + public override bool NeedsDrain => _connection != null && _connection._disposed != Status_Disposed; public override async ValueTask DrainAsync(int maxDrainBytes) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs index dd667f0706030d..f5eee3b7bde56c 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs @@ -190,7 +190,9 @@ private ReadOnlyMemory ReadFromConnectionBuffer(int maxBytesToRead) return connectionBuffer.Slice(0, bytesToConsume); } - public override bool NeedsDrain => (_connection != null); + // _connection == null typically means that we have finished reading the response. + // Cancellation may lead to a state where a disposed _connection is not null. + public override bool NeedsDrain => _connection != null && _connection._disposed != Status_Disposed; public override async ValueTask DrainAsync(int maxDrainBytes) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs index 8ae577dfaf844c..4daad8f2b0ec76 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs @@ -46,8 +46,7 @@ protected override void Dispose(bool disposing) return; } - // The connection might be disposed because of cancellation. We should not drain the response in this case. - if (disposing && NeedsDrain && _connection?._disposed != Status_Disposed) + if (disposing && NeedsDrain) { // Start the asynchronous drain. // It may complete synchronously, in which case the connection will be put back in the pool synchronously. From bf57a3dfb7a2679e0771cf13f2be147a088a1197 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Fri, 13 Aug 2021 16:17:25 +0200 Subject: [PATCH 3/4] safe check of _connection._disposed --- .../SocketsHttpHandler/ChunkedEncodingReadStream.cs | 4 +--- .../SocketsHttpHandler/ContentLengthReadStream.cs | 4 +--- .../Http/SocketsHttpHandler/HttpContentReadStream.cs | 11 +++++++++++ 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs index d8c153be3fd4c6..f76f3a818b20c6 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs @@ -425,9 +425,7 @@ private enum ParsingState : byte Done } - // _connection == null typically means that we have finished reading the response. - // Cancellation may lead to a state where a disposed _connection is not null. - public override bool NeedsDrain => _connection != null && _connection._disposed != Status_Disposed; + public override bool NeedsDrain => IsConnectionAlive; public override async ValueTask DrainAsync(int maxDrainBytes) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs index f5eee3b7bde56c..e11f8240e77a3c 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs @@ -190,9 +190,7 @@ private ReadOnlyMemory ReadFromConnectionBuffer(int maxBytesToRead) return connectionBuffer.Slice(0, bytesToConsume); } - // _connection == null typically means that we have finished reading the response. - // Cancellation may lead to a state where a disposed _connection is not null. - public override bool NeedsDrain => _connection != null && _connection._disposed != Status_Disposed; + public override bool NeedsDrain => IsConnectionAlive; public override async ValueTask DrainAsync(int maxDrainBytes) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs index 4daad8f2b0ec76..5c8b5ca21dffc2 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs @@ -28,6 +28,17 @@ public HttpContentReadStream(HttpConnection connection) : base(connection) protected bool IsDisposed => _disposed == 1; + protected bool IsConnectionAlive + { + get + { + // _connection == null typically means that we have finished reading the response. + // Cancellation may lead to a state where a disposed _connection is not null. + HttpConnection? connection = _connection; + return connection != null && connection._disposed != Status_Disposed; + } + } + public virtual ValueTask DrainAsync(int maxDrainBytes) { Debug.Fail($"DrainAsync should not be called for this response stream: {GetType()}"); From 57ed154fd3b4ae99fb203fa59a35b75f5c2a4280 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Fri, 13 Aug 2021 16:33:29 +0200 Subject: [PATCH 4/4] IsConnectionAlive -> CanReadFromConnection --- .../Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs | 2 +- .../Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs | 2 +- .../System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs index f76f3a818b20c6..ff87d5a89f9432 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ChunkedEncodingReadStream.cs @@ -425,7 +425,7 @@ private enum ParsingState : byte Done } - public override bool NeedsDrain => IsConnectionAlive; + public override bool NeedsDrain => CanReadFromConnection; public override async ValueTask DrainAsync(int maxDrainBytes) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs index e11f8240e77a3c..786f285a93c75e 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthReadStream.cs @@ -190,7 +190,7 @@ private ReadOnlyMemory ReadFromConnectionBuffer(int maxBytesToRead) return connectionBuffer.Slice(0, bytesToConsume); } - public override bool NeedsDrain => IsConnectionAlive; + public override bool NeedsDrain => CanReadFromConnection; public override async ValueTask DrainAsync(int maxDrainBytes) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs index 5c8b5ca21dffc2..02ba78ab99ca93 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs @@ -28,7 +28,7 @@ public HttpContentReadStream(HttpConnection connection) : base(connection) protected bool IsDisposed => _disposed == 1; - protected bool IsConnectionAlive + protected bool CanReadFromConnection { get {