From b21ca7216c79068ccad4c8a8949c3dbdbc3f7651 Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Sun, 26 Jan 2020 00:05:18 +0000 Subject: [PATCH 1/9] Add Read Span overload to HttpRequestStreamReader --- .../src/HttpRequestStreamReader.cs | 53 +++++++++++++++++- .../test/HttpRequestStreamReaderTest.cs | 54 ++++++++++++++++++- 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs index 3f9478c5deaa..2e32ebb55a8d 100644 --- a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs +++ b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs @@ -200,6 +200,57 @@ public override int Read(char[] buffer, int index, int count) return charsRead; } + public override int Read(Span buffer) + { + if (buffer == null) + { + throw new ArgumentNullException(nameof(buffer)); + } + + var count = buffer.Length; + var charsRead = 0; + while (count > 0) + { + var charsRemaining = _charsRead - _charBufferIndex; + if (charsRemaining == 0) + { + charsRemaining = ReadIntoBuffer(); + } + + if (charsRemaining == 0) + { + break; // We're at EOF + } + + if (charsRemaining > count) + { + charsRemaining = count; + } + + var source = new ReadOnlySpan(_charBuffer, _charBufferIndex, charsRemaining); + source.CopyTo(buffer); + + _charBufferIndex += charsRemaining; + + charsRead += charsRemaining; + count -= charsRemaining; + + if (count > 0) + { + buffer = buffer.Slice(charsRemaining, count); + } + + // If we got back fewer chars than we asked for, then it's likely the underlying stream is blocked. + // Send the data back to the caller so they can process it. + if (_isBlocked) + { + break; + } + } + + return charsRead; + } + public override async Task ReadAsync(char[] buffer, int index, int count) { if (buffer == null) @@ -371,4 +422,4 @@ private async Task ReadIntoBufferAsync() return _charsRead; } } -} \ No newline at end of file +} diff --git a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs index 6cab20bccc7f..0a43214dc660 100644 --- a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs +++ b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs @@ -195,6 +195,44 @@ public static async Task ReadLineAsync_MultipleContinuousLines() Assert.Null(eol); } + [Fact] + public static void Read_Span_ReadAllCharactersAtOnce() + { + // Arrange + var reader = CreateReader(); + var chars = new char[CharData.Length]; + var span = new Span(chars); + + // Act + var read = reader.Read(span); + + // Assert + Assert.Equal(chars.Length, read); + for (var i = 0; i < CharData.Length; i++) + { + Assert.Equal(CharData[i], chars[i]); + } + } + + [Fact] + public static void Read_Span_WithMoreDataThanInternalBufferSize() + { + // Arrange + var reader = CreateReader(10); + var chars = new char[CharData.Length]; + var span = new Span(chars); + + // Act + var read = reader.Read(span); + + // Assert + Assert.Equal(chars.Length, read); + for (var i = 0; i < CharData.Length; i++) + { + Assert.Equal(CharData[i], chars[i]); + } + } + [Theory] [MemberData(nameof(HttpRequestNullData))] public static void NullInputsInConstructor_ExpectArgumentNullException(Stream stream, Encoding encoding, ArrayPool bytePool, ArrayPool charPool) @@ -253,15 +291,27 @@ await Assert.ThrowsAsync(() => return httpRequestStreamReader.ReadAsync(new char[10], 0, 1); }); } + private static HttpRequestStreamReader CreateReader() + { + MemoryStream stream = CreateStream(); + return new HttpRequestStreamReader(stream, Encoding.UTF8); + } + + private static HttpRequestStreamReader CreateReader(int bufferSize) + { + MemoryStream stream = CreateStream(); + return new HttpRequestStreamReader(stream, Encoding.UTF8, bufferSize); + } + + private static MemoryStream CreateStream() { var stream = new MemoryStream(); var writer = new StreamWriter(stream); writer.Write(CharData); writer.Flush(); stream.Position = 0; - - return new HttpRequestStreamReader(stream, Encoding.UTF8); + return stream; } private static MemoryStream GetSmallStream() From 5318b1f92caaadbe22c59a0065912063507b6c71 Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Mon, 3 Feb 2020 23:26:37 +0000 Subject: [PATCH 2/9] ReadAsync for Memory of char override --- .../src/HttpRequestStreamReader.cs | 53 +++++++++++-------- .../test/HttpRequestStreamReaderTest.cs | 42 ++++++++++++++- 2 files changed, 72 insertions(+), 23 deletions(-) diff --git a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs index 2e32ebb55a8d..e92c1e17632d 100644 --- a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs +++ b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs @@ -6,6 +6,7 @@ using System.Diagnostics; using System.IO; using System.Text; +using System.Threading; using System.Threading.Tasks; namespace Microsoft.AspNetCore.WebUtilities @@ -251,7 +252,7 @@ public override int Read(Span buffer) return charsRead; } - public override async Task ReadAsync(char[] buffer, int index, int count) + public override Task ReadAsync(char[] buffer, int index, int count) { if (buffer == null) { @@ -268,6 +269,12 @@ public override async Task ReadAsync(char[] buffer, int index, int count) throw new ArgumentOutOfRangeException(nameof(count)); } + var memory = new Memory(buffer, index, count); + return ReadAsync(memory).AsTask(); + } + + public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { if (_disposed) { throw new ObjectDisposedException(nameof(HttpRequestStreamReader)); @@ -278,14 +285,16 @@ public override async Task ReadAsync(char[] buffer, int index, int count) return 0; } + var count = buffer.Length; + var charsRead = 0; while (count > 0) { // n is the characters available in _charBuffer - var n = _charsRead - _charBufferIndex; + var charsAvailable = _charsRead - _charBufferIndex; // charBuffer is empty, let's read from the stream - if (n == 0) + if (charsAvailable == 0) { _charsRead = 0; _charBufferIndex = 0; @@ -295,7 +304,7 @@ public override async Task ReadAsync(char[] buffer, int index, int count) // We break out of the loop if the stream is blocked (EOF is reached). do { - Debug.Assert(n == 0); + Debug.Assert(charsAvailable == 0); _bytesRead = await _stream.ReadAsync( _byteBuffer, 0, @@ -309,45 +318,47 @@ public override async Task ReadAsync(char[] buffer, int index, int count) // _isBlocked == whether we read fewer bytes than we asked for. _isBlocked = (_bytesRead < _byteBufferSize); - Debug.Assert(n == 0); + Debug.Assert(charsAvailable == 0); _charBufferIndex = 0; - n = _decoder.GetChars( + charsAvailable = _decoder.GetChars( _byteBuffer, 0, _bytesRead, _charBuffer, 0); - Debug.Assert(n > 0); + Debug.Assert(charsAvailable > 0); - _charsRead += n; // Number of chars in StreamReader's buffer. + _charsRead += charsAvailable; // Number of chars in StreamReader's buffer. } - while (n == 0); + while (charsAvailable == 0); - if (n == 0) + if (charsAvailable == 0) { break; // We're at EOF } } // Got more chars in charBuffer than the user requested - if (n > count) + if (charsAvailable > count) { - n = count; + charsAvailable = count; } - Buffer.BlockCopy( - _charBuffer, - _charBufferIndex * 2, - buffer, - (index + charsRead) * 2, - n * 2); + var source = new Memory(_charBuffer, _charBufferIndex, charsAvailable); + source.CopyTo(buffer); + + if (charsAvailable < count) + { + // update the buffer to the remaining portion + buffer = buffer.Slice(charsAvailable); + } - _charBufferIndex += n; + _charBufferIndex += charsAvailable; - charsRead += n; - count -= n; + charsRead += charsAvailable; + count -= charsAvailable; // This function shouldn't block for an indefinite amount of time, // or reading from a network stream won't work right. If we got diff --git a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs index 0a43214dc660..710db6d0734b 100644 --- a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs +++ b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs @@ -13,7 +13,7 @@ namespace Microsoft.AspNetCore.WebUtilities { - public class HttpResponseStreamReaderTest + public class HttpRequestStreamReaderTest { private static readonly char[] CharData = new char[] { @@ -118,7 +118,7 @@ public static void Read_ReadAllCharactersAtOnce() } [Fact] - public static async Task Read_ReadInTwoChunks() + public static async Task ReadAsync_ReadInTwoChunks() { // Arrange var reader = CreateReader(); @@ -233,6 +233,44 @@ public static void Read_Span_WithMoreDataThanInternalBufferSize() } } + [Fact] + public async static Task ReadAsync_Memory_ReadAllCharactersAtOnce() + { + // Arrange + var reader = CreateReader(); + var chars = new char[CharData.Length]; + var memory = new Memory(chars); + + // Act + var read = await reader.ReadAsync(memory); + + // Assert + Assert.Equal(chars.Length, read); + for (var i = 0; i < CharData.Length; i++) + { + Assert.Equal(CharData[i], chars[i]); + } + } + + [Fact] + public async static Task ReadAsync_Memory_WithMoreDataThanInternalBufferSize() + { + // Arrange + var reader = CreateReader(10); + var chars = new char[CharData.Length]; + var memory = new Memory(chars); + + // Act + var read = await reader.ReadAsync(memory); + + // Assert + Assert.Equal(chars.Length, read); + for (var i = 0; i < CharData.Length; i++) + { + Assert.Equal(CharData[i], chars[i]); + } + } + [Theory] [MemberData(nameof(HttpRequestNullData))] public static void NullInputsInConstructor_ExpectArgumentNullException(Stream stream, Encoding encoding, ArrayPool bytePool, ArrayPool charPool) From ecae87587e403e425e3f71b1bed1c5c75e243fb3 Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Mon, 3 Feb 2020 23:38:38 +0000 Subject: [PATCH 3/9] Refactored Read Span and added tests for dispose --- .../src/HttpRequestStreamReader.cs | 51 +++---------------- .../test/HttpRequestStreamReaderTest.cs | 26 +++++++--- 2 files changed, 27 insertions(+), 50 deletions(-) diff --git a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs index e92c1e17632d..0854b3aae441 100644 --- a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs +++ b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs @@ -155,50 +155,8 @@ public override int Read(char[] buffer, int index, int count) throw new ArgumentOutOfRangeException(nameof(count)); } - if (_disposed) - { - throw new ObjectDisposedException(nameof(HttpRequestStreamReader)); - } - - var charsRead = 0; - while (count > 0) - { - var charsRemaining = _charsRead - _charBufferIndex; - if (charsRemaining == 0) - { - charsRemaining = ReadIntoBuffer(); - } - - if (charsRemaining == 0) - { - break; // We're at EOF - } - - if (charsRemaining > count) - { - charsRemaining = count; - } - - Buffer.BlockCopy( - _charBuffer, - _charBufferIndex * 2, - buffer, - (index + charsRead) * 2, - charsRemaining * 2); - _charBufferIndex += charsRemaining; - - charsRead += charsRemaining; - count -= charsRemaining; - - // If we got back fewer chars than we asked for, then it's likely the underlying stream is blocked. - // Send the data back to the caller so they can process it. - if (_isBlocked) - { - break; - } - } - - return charsRead; + var span = new Span(buffer, index, count); + return Read(span); } public override int Read(Span buffer) @@ -208,6 +166,11 @@ public override int Read(Span buffer) throw new ArgumentNullException(nameof(buffer)); } + if (_disposed) + { + throw new ObjectDisposedException(nameof(HttpRequestStreamReader)); + } + var count = buffer.Length; var charsRead = 0; while (count > 0) diff --git a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs index 710db6d0734b..c806244ce1a0 100644 --- a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs +++ b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs @@ -318,16 +318,14 @@ public static void StreamDisposed_ExpectedObjectDisposedException(Action action) { var httpRequestStreamReader = new HttpRequestStreamReader(new MemoryStream(), Encoding.UTF8, 10, ArrayPool.Shared, ArrayPool.Shared); httpRequestStreamReader.Dispose(); - await Assert.ThrowsAsync(() => - { - return httpRequestStreamReader.ReadAsync(new char[10], 0, 1); - }); + await Assert.ThrowsAsync(() => action(httpRequestStreamReader)); } private static HttpRequestStreamReader CreateReader() @@ -390,6 +388,10 @@ public static IEnumerable HttpRequestDisposeData() { var res = httpRequestStreamReader.Read(new char[10], 0, 1); })}; + yield return new object[] { new Action((httpRequestStreamReader) => + { + var res = httpRequestStreamReader.Read(new Span(new char[10], 0, 1)); + })}; yield return new object[] { new Action((httpRequestStreamReader) => { @@ -397,5 +399,17 @@ public static IEnumerable HttpRequestDisposeData() })}; } + + public static IEnumerable HttpRequestDisposeDataAsync() + { + yield return new object[] { new Func(async (httpRequestStreamReader) => + { + await httpRequestStreamReader.ReadAsync(new char[10], 0, 1); + })}; + yield return new object[] { new Func(async (httpRequestStreamReader) => + { + await httpRequestStreamReader.ReadAsync(new Memory(new char[10], 0, 1)); + })}; + } } } From 0229019172411260f9c30539c7681bbaac50ebea Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Tue, 4 Feb 2020 23:57:28 +0000 Subject: [PATCH 4/9] Implemented ReadLineAsync --- .../src/HttpRequestStreamReader.cs | 52 ++++++++++++++++++- .../test/HttpRequestStreamReaderTest.cs | 45 ++++++++++------ 2 files changed, 79 insertions(+), 18 deletions(-) diff --git a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs index 0854b3aae441..92af85e464bd 100644 --- a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs +++ b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs @@ -335,6 +335,57 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation return charsRead; } + public override async Task ReadLineAsync() + { + if (_disposed) + { + throw new ObjectDisposedException(nameof(HttpRequestStreamReader)); + } + + StringBuilder sb = new StringBuilder(); + while (true) + { + if (_charBufferIndex == _charsRead) + { + if (await ReadIntoBufferAsync() == 0) + { + break; // reached EOF, we need to return null if we were at EOF from the beginning + } + } + + var ch = _charBuffer[_charBufferIndex++]; + + if (ch == '\r' || ch == '\n') + { + if (ch == '\r') + { + if (_charBufferIndex == _charsRead) + { + if (await ReadIntoBufferAsync() == 0) + { + return sb.ToString(); // reached EOF + } + } + + if (_charBuffer[_charBufferIndex] == '\n') + { + _charBufferIndex++; // consume the \n character + } + } + + return sb.ToString(); + } + sb.Append(ch); + } + + if (sb.Length > 0) + { + return sb.ToString(); + } + + return null; + } + private int ReadIntoBuffer() { _charsRead = 0; @@ -370,7 +421,6 @@ private async Task ReadIntoBufferAsync() do { - _bytesRead = await _stream.ReadAsync( _byteBuffer, 0, diff --git a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs index c806244ce1a0..07c6ec4f31a0 100644 --- a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs +++ b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs @@ -135,29 +135,31 @@ public static async Task ReadAsync_ReadInTwoChunks() } } - [Fact] - public static void ReadLine_ReadMultipleLines() + [Theory] + [MemberData(nameof(ReadLineData))] + public static async Task ReadLine_ReadMultipleLines(Func> action) { // Arrange var reader = CreateReader(); var valueString = new string(CharData); // Act & Assert - var data = reader.ReadLine(); + var data = await action(reader); Assert.Equal(valueString.Substring(0, valueString.IndexOf('\r')), data); - data = reader.ReadLine(); + data = await action(reader); Assert.Equal(valueString.Substring(valueString.IndexOf('\r') + 1, 3), data); - data = reader.ReadLine(); + data = await action(reader); Assert.Equal(valueString.Substring(valueString.IndexOf('\n') + 1, 2), data); - data = reader.ReadLine(); + data = await action(reader); Assert.Equal((valueString.Substring(valueString.LastIndexOf('\n') + 1)), data); } - [Fact] - public static void ReadLine_ReadWithNoNewlines() + [Theory] + [MemberData(nameof(ReadLineData))] + public static async Task ReadLine_ReadWithNoNewlines(Func> action) { // Arrange var reader = CreateReader(); @@ -166,32 +168,33 @@ public static void ReadLine_ReadWithNoNewlines() // Act reader.Read(temp, 0, 1); - var data = reader.ReadLine(); + var data = await action(reader); // Assert Assert.Equal(valueString.Substring(1, valueString.IndexOf('\r') - 1), data); } - [Fact] - public static async Task ReadLineAsync_MultipleContinuousLines() + [Theory] + [MemberData(nameof(ReadLineData))] + public static async Task ReadLine_MultipleContinuousLines(Func> action) { // Arrange var stream = new MemoryStream(); var writer = new StreamWriter(stream); - writer.Write("\n\n\r\r\n"); + writer.Write("\n\n\r\r\n\r"); writer.Flush(); stream.Position = 0; var reader = new HttpRequestStreamReader(stream, Encoding.UTF8); // Act & Assert - for (var i = 0; i < 4; i++) + for (var i = 0; i < 5; i++) { - var data = await reader.ReadLineAsync(); + var data = await action(reader); Assert.Equal(string.Empty, data); } - var eol = await reader.ReadLineAsync(); + var eol = await action(reader); Assert.Null(eol); } @@ -281,8 +284,6 @@ public static void NullInputsInConstructor_ExpectArgumentNullException(Stream st }); } - - [Theory] [InlineData(0)] [InlineData(-1)] @@ -411,5 +412,15 @@ public static IEnumerable HttpRequestDisposeDataAsync() await httpRequestStreamReader.ReadAsync(new Memory(new char[10], 0, 1)); })}; } + + public static IEnumerable ReadLineData() + { + yield return new object[] { new Func>((httpRequestStreamReader) => + httpRequestStreamReader.ReadLineAsync() + )}; + yield return new object[] { new Func>((httpRequestStreamReader) => + Task.FromResult(httpRequestStreamReader.ReadLine()) + )}; + } } } From cd7538b46d8cf88fbf8e57e81d982b454c474c9e Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Wed, 5 Feb 2020 00:08:53 +0000 Subject: [PATCH 5/9] Updated reference assembly --- .../ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs b/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs index 3aa6ce55dc23..ef4dc9919fe2 100644 --- a/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs +++ b/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs @@ -145,8 +145,12 @@ protected override void Dispose(bool disposing) { } public override int Peek() { throw null; } public override int Read() { throw null; } public override int Read(char[] buffer, int index, int count) { throw null; } - [System.Diagnostics.DebuggerStepThroughAttribute] + public override int Read(System.Span buffer) { throw null; } public override System.Threading.Tasks.Task ReadAsync(char[] buffer, int index, int count) { throw null; } + [System.Diagnostics.DebuggerStepThroughAttribute] + public override System.Threading.Tasks.ValueTask ReadAsync(System.Memory buffer, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } + [System.Diagnostics.DebuggerStepThroughAttribute] + public override System.Threading.Tasks.Task ReadLineAsync() { throw null; } } public partial class HttpResponseStreamWriter : System.IO.TextWriter { From e3b3db32269eed68a6fed7711e76097570ac0192 Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Mon, 17 Feb 2020 00:27:04 +0000 Subject: [PATCH 6/9] ReadLine reimplementation --- ...ttpRequestStreamReaderReadLineBenchmark.cs | 54 +++++++ ...AspNetCore.WebUtilities.Performance.csproj | 2 +- .../src/HttpRequestStreamReader.cs | 147 +++++++++++++++--- .../test/HttpRequestStreamReaderTest.cs | 63 +++++++- 4 files changed, 240 insertions(+), 26 deletions(-) create mode 100644 src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs diff --git a/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs new file mode 100644 index 000000000000..ac70e1b3ffba --- /dev/null +++ b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs @@ -0,0 +1,54 @@ +using System.Diagnostics; +using System.IO; +using System.Text; +using System.Threading.Tasks; +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Attributes.Jobs; + +namespace Microsoft.AspNetCore.WebUtilities +{ + [SimpleJob] + public class HttpRequestStreamReaderReadLineBenchmark + { + private MemoryStream _stream; + + [Params(200, 1000, 1025, 1600)] // Default buffer length is 1024 + public int Length { get; set; } + + [GlobalSetup] + public void GlobalSetup() + { + var data = new char[Length]; + + data[Length - 2] = '\r'; + data[Length - 1] = '\n'; + + _stream = new MemoryStream(Encoding.UTF8.GetBytes(data)); + } + + [Benchmark] + public async Task ReadLineAsync() + { + var reader = CreateReader(); + var result = await reader.ReadLineAsync(); + Debug.Assert(result.Length == Length - 2); + return result; + } + + [Benchmark] + public string ReadLine() + { + var reader = CreateReader(); + var result = reader.ReadLine(); + Debug.Assert(result.Length == Length - 2); + return result; + } + + [Benchmark] + public HttpRequestStreamReader CreateReader() + { + _stream.Seek(0, SeekOrigin.Begin); + return new HttpRequestStreamReader(_stream, Encoding.UTF8); + } + } +} diff --git a/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/Microsoft.AspNetCore.WebUtilities.Performance.csproj b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/Microsoft.AspNetCore.WebUtilities.Performance.csproj index a170bf6cc118..4744da361b8a 100644 --- a/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/Microsoft.AspNetCore.WebUtilities.Performance.csproj +++ b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/Microsoft.AspNetCore.WebUtilities.Performance.csproj @@ -1,4 +1,4 @@ - + $(DefaultNetCoreTargetFramework) diff --git a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs index 92af85e464bd..6ed4347f6b41 100644 --- a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs +++ b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs @@ -340,50 +340,138 @@ public override async Task ReadLineAsync() if (_disposed) { throw new ObjectDisposedException(nameof(HttpRequestStreamReader)); - } + } + + StringBuilder sb = null; + var consumeLineFeed = false; - StringBuilder sb = new StringBuilder(); while (true) { if (_charBufferIndex == _charsRead) { if (await ReadIntoBufferAsync() == 0) { - break; // reached EOF, we need to return null if we were at EOF from the beginning + // reached EOF, we need to return null if we were at EOF from the beginning + return sb?.ToString(); } } - var ch = _charBuffer[_charBufferIndex++]; - - if (ch == '\r' || ch == '\n') + var stepResult = ReadLineStep(ref sb, ref consumeLineFeed); + + if (stepResult.Completed) { - if (ch == '\r') + return stepResult.Result ?? sb?.ToString(); + } + + continue; + } + } + + // Reads a line. A line is defined as a sequence of characters followed by + // a carriage return ('\r'), a line feed ('\n'), or a carriage return + // immediately followed by a line feed. The resulting string does not + // contain the terminating carriage return and/or line feed. The returned + // value is null if the end of the input stream has been reached. + public override string ReadLine() + { + if (_disposed) + { + throw new ObjectDisposedException(nameof(HttpRequestStreamReader)); + } + + StringBuilder sb = null; + var consumeLineFeed = false; + + while (true) + { + if (_charBufferIndex == _charsRead) + { + if (ReadIntoBuffer() == 0) { - if (_charBufferIndex == _charsRead) + // reached EOF, we need to return null if we were at EOF from the beginning + return sb?.ToString(); + } + } + + var stepResult = ReadLineStep(ref sb, ref consumeLineFeed); + + if (stepResult.Completed) + { + return stepResult.Result ?? sb?.ToString(); + } + + continue; + } + } + + private ReadLineStepResult ReadLineStep(ref StringBuilder sb, ref bool consumeLineFeed) + { + if (consumeLineFeed) + { + if (_charBuffer[_charBufferIndex] == '\n') + { + _charBufferIndex++; + } + return ReadLineStepResult.Done; + } + + var span = new Span(_charBuffer, _charBufferIndex, _charsRead - _charBufferIndex); + + var index = span.IndexOfAny('\r', '\n'); + + if (index != -1) + { + if (span[index] == '\r') + { + span = span.Slice(0, index); + _charBufferIndex += index + 1; + + if (_charBufferIndex < _charsRead) + { + // consume following \n + if (_charBuffer[_charBufferIndex] == '\n') { - if (await ReadIntoBufferAsync() == 0) - { - return sb.ToString(); // reached EOF - } + _charBufferIndex++; } - if (_charBuffer[_charBufferIndex] == '\n') + if (sb != null) { - _charBufferIndex++; // consume the \n character + sb.Append(span); + return ReadLineStepResult.Done; } + + // perf: if the new line is found in first pass, we skip the StringBuilder + return ReadLineStepResult.FromResult(span.ToString()); } - return sb.ToString(); + // we where at the end of buffer, we need to read more to check for a line feed to consume + sb ??= new StringBuilder(); + sb.Append(span); + consumeLineFeed = true; + return ReadLineStepResult.Continue; } - sb.Append(ch); - } - if (sb.Length > 0) - { - return sb.ToString(); + if (span[index] == '\n') + { + span = span.Slice(0, index); + _charBufferIndex += index + 1; + + if (sb != null) + { + sb.Append(span); + return ReadLineStepResult.Done; + } + + // perf: if the new line is found in first pass, we skip the StringBuilder + return ReadLineStepResult.FromResult(span.ToString()); + } } - return null; + sb ??= new StringBuilder(); + sb.Append(span); + _charBufferIndex = _charsRead; + + return ReadLineStepResult.Continue; } private int ReadIntoBuffer() @@ -445,5 +533,22 @@ private async Task ReadIntoBufferAsync() return _charsRead; } + + private readonly struct ReadLineStepResult + { + public static readonly ReadLineStepResult Done = new ReadLineStepResult(true, null); + public static readonly ReadLineStepResult Continue = new ReadLineStepResult(false, null); + + public static ReadLineStepResult FromResult(string value) => new ReadLineStepResult(true, value); + + private ReadLineStepResult(bool completed, string result) + { + Completed = completed; + Result = result; + } + + public bool Completed { get; } + public string Result { get; } + } } } diff --git a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs index 07c6ec4f31a0..5d65accbe0c9 100644 --- a/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs +++ b/src/Http/WebUtilities/test/HttpRequestStreamReaderTest.cs @@ -194,8 +194,63 @@ public static async Task ReadLine_MultipleContinuousLines(Func> action) + { + // Arrange + var stream = new MemoryStream(); + var writer = new StreamWriter(stream); + writer.Write("123456789\r\nfoo"); + writer.Flush(); + stream.Position = 0; + + var reader = new HttpRequestStreamReader(stream, Encoding.UTF8, 10); + + // Act & Assert + var data = await action(reader); + Assert.Equal("123456789", data); + + data = await action(reader); + Assert.Equal("foo", data); + + var eof = await action(reader); + Assert.Null(eof); + } + + [Theory] + [MemberData(nameof(ReadLineData))] + public static async Task ReadLine_EOF(Func> action) + { + // Arrange + var stream = new MemoryStream(); + var reader = new HttpRequestStreamReader(stream, Encoding.UTF8); + + // Act & Assert + var eof = await action(reader); + Assert.Null(eof); + } + + [Theory] + [MemberData(nameof(ReadLineData))] + public static async Task ReadLine_NewLineOnly(Func> action) + { + // Arrange + var stream = new MemoryStream(); + var writer = new StreamWriter(stream); + writer.Write("\r\n"); + writer.Flush(); + stream.Position = 0; + + var reader = new HttpRequestStreamReader(stream, Encoding.UTF8); + + // Act & Assert + var empty = await action(reader); + Assert.Equal(string.Empty, empty); } [Fact] @@ -416,10 +471,10 @@ public static IEnumerable HttpRequestDisposeDataAsync() public static IEnumerable ReadLineData() { yield return new object[] { new Func>((httpRequestStreamReader) => - httpRequestStreamReader.ReadLineAsync() + Task.FromResult(httpRequestStreamReader.ReadLine()) )}; yield return new object[] { new Func>((httpRequestStreamReader) => - Task.FromResult(httpRequestStreamReader.ReadLine()) + httpRequestStreamReader.ReadLineAsync() )}; } } From e48e7e1dc1a79b7ba15078e2d16292764ed5ea62 Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Mon, 2 Mar 2020 13:34:58 +0000 Subject: [PATCH 7/9] Updated reference assemblies --- .../ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs b/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs index ef4dc9919fe2..4417d47e821a 100644 --- a/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs +++ b/src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp.cs @@ -149,6 +149,7 @@ protected override void Dispose(bool disposing) { } public override System.Threading.Tasks.Task ReadAsync(char[] buffer, int index, int count) { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute] public override System.Threading.Tasks.ValueTask ReadAsync(System.Memory buffer, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } + public override string ReadLine() { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute] public override System.Threading.Tasks.Task ReadLineAsync() { throw null; } } From 327aa1f7e285170ea04808701dcf9ae478f193a2 Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Tue, 7 Apr 2020 09:13:22 +0100 Subject: [PATCH 8/9] Feedback --- .../src/HttpRequestStreamReader.cs | 58 +++++++++---------- 1 file changed, 26 insertions(+), 32 deletions(-) diff --git a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs index 6ed4347f6b41..c500fa297840 100644 --- a/src/Http/WebUtilities/src/HttpRequestStreamReader.cs +++ b/src/Http/WebUtilities/src/HttpRequestStreamReader.cs @@ -199,10 +199,7 @@ public override int Read(Span buffer) charsRead += charsRemaining; count -= charsRemaining; - if (count > 0) - { - buffer = buffer.Slice(charsRemaining, count); - } + buffer = buffer.Slice(charsRemaining, count); // If we got back fewer chars than we asked for, then it's likely the underlying stream is blocked. // Send the data back to the caller so they can process it. @@ -254,10 +251,10 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation while (count > 0) { // n is the characters available in _charBuffer - var charsAvailable = _charsRead - _charBufferIndex; + var charsRemaining = _charsRead - _charBufferIndex; // charBuffer is empty, let's read from the stream - if (charsAvailable == 0) + if (charsRemaining == 0) { _charsRead = 0; _charBufferIndex = 0; @@ -267,7 +264,7 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation // We break out of the loop if the stream is blocked (EOF is reached). do { - Debug.Assert(charsAvailable == 0); + Debug.Assert(charsRemaining == 0); _bytesRead = await _stream.ReadAsync( _byteBuffer, 0, @@ -281,47 +278,43 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation // _isBlocked == whether we read fewer bytes than we asked for. _isBlocked = (_bytesRead < _byteBufferSize); - Debug.Assert(charsAvailable == 0); + Debug.Assert(charsRemaining == 0); _charBufferIndex = 0; - charsAvailable = _decoder.GetChars( + charsRemaining = _decoder.GetChars( _byteBuffer, 0, _bytesRead, _charBuffer, 0); - Debug.Assert(charsAvailable > 0); + Debug.Assert(charsRemaining > 0); - _charsRead += charsAvailable; // Number of chars in StreamReader's buffer. + _charsRead += charsRemaining; // Number of chars in StreamReader's buffer. } - while (charsAvailable == 0); + while (charsRemaining == 0); - if (charsAvailable == 0) + if (charsRemaining == 0) { break; // We're at EOF } } // Got more chars in charBuffer than the user requested - if (charsAvailable > count) + if (charsRemaining > count) { - charsAvailable = count; + charsRemaining = count; } - var source = new Memory(_charBuffer, _charBufferIndex, charsAvailable); + var source = new Memory(_charBuffer, _charBufferIndex, charsRemaining); source.CopyTo(buffer); - if (charsAvailable < count) - { - // update the buffer to the remaining portion - buffer = buffer.Slice(charsAvailable); - } + _charBufferIndex += charsRemaining; - _charBufferIndex += charsAvailable; + charsRead += charsRemaining; + count -= charsRemaining; - charsRead += charsAvailable; - count -= charsAvailable; + buffer = buffer.Slice(charsRemaining, count); // This function shouldn't block for an indefinite amount of time, // or reading from a network stream won't work right. If we got @@ -399,16 +392,17 @@ public override string ReadLine() { return stepResult.Result ?? sb?.ToString(); } - - continue; } } private ReadLineStepResult ReadLineStep(ref StringBuilder sb, ref bool consumeLineFeed) { + const char carriageReturn = '\r'; + const char lineFeed = '\n'; + if (consumeLineFeed) { - if (_charBuffer[_charBufferIndex] == '\n') + if (_charBuffer[_charBufferIndex] == lineFeed) { _charBufferIndex++; } @@ -417,19 +411,19 @@ private ReadLineStepResult ReadLineStep(ref StringBuilder sb, ref bool consumeLi var span = new Span(_charBuffer, _charBufferIndex, _charsRead - _charBufferIndex); - var index = span.IndexOfAny('\r', '\n'); + var index = span.IndexOfAny(carriageReturn, lineFeed); if (index != -1) { - if (span[index] == '\r') + if (span[index] == carriageReturn) { span = span.Slice(0, index); _charBufferIndex += index + 1; if (_charBufferIndex < _charsRead) { - // consume following \n - if (_charBuffer[_charBufferIndex] == '\n') + // consume following line feed + if (_charBuffer[_charBufferIndex] == lineFeed) { _charBufferIndex++; } @@ -451,7 +445,7 @@ private ReadLineStepResult ReadLineStep(ref StringBuilder sb, ref bool consumeLi return ReadLineStepResult.Continue; } - if (span[index] == '\n') + if (span[index] == lineFeed) { span = span.Slice(0, index); _charBufferIndex += index + 1; From 5c3a347d1f3b76d05fd1bf263ac86d97175c5525 Mon Sep 17 00:00:00 2001 From: Alessio Franceschelli Date: Sun, 12 Apr 2020 11:32:34 +0100 Subject: [PATCH 9/9] Fixed benchmark for master changes --- .../HttpRequestStreamReaderReadLineBenchmark.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs index ac70e1b3ffba..49184ed8e1a5 100644 --- a/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs +++ b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/HttpRequestStreamReaderReadLineBenchmark.cs @@ -3,11 +3,9 @@ using System.Text; using System.Threading.Tasks; using BenchmarkDotNet.Attributes; -using BenchmarkDotNet.Attributes.Jobs; namespace Microsoft.AspNetCore.WebUtilities { - [SimpleJob] public class HttpRequestStreamReaderReadLineBenchmark { private MemoryStream _stream;