From 4b4bc2d49cd6ea1039b75067437988ac6134b4b9 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 16 Mar 2018 15:41:01 -0700 Subject: [PATCH 1/3] Disable flaky test SynchronousReadAndWriteTests.ReadAndWriteEchoTwice (#688) --- .../Inprocess/SynchronousReadAndWriteTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/IISIntegration.FunctionalTests/Inprocess/SynchronousReadAndWriteTests.cs b/test/IISIntegration.FunctionalTests/Inprocess/SynchronousReadAndWriteTests.cs index c79c8a462c..e69437a7b8 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/SynchronousReadAndWriteTests.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/SynchronousReadAndWriteTests.cs @@ -58,7 +58,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests Assert.Equal(body, responseText); } - [ConditionalFact] + [ConditionalFact(Skip = "See https://github.com/aspnet/IISIntegration/issues/687")] public async Task ReadAndWriteEchoTwice() { var requestBody = new string('a', 10000); From cf4874997eec8ce11a66b03856772737dec5a4c4 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 16 Mar 2018 16:56:23 -0700 Subject: [PATCH 2/3] Heap allocate HTTP_DATA_CHUNK if the size of the response is too large. (#683) --- .../Server/IISHttpContext.ReadWrite.cs | 76 ++++++++++++------- .../Inprocess/LargeResponseBodyTests.cs | 10 ++- test/IISTestSite/Startup.cs | 22 ++++++ 3 files changed, 80 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs index 89266d9e9a..5329239167 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs @@ -12,6 +12,8 @@ namespace Microsoft.AspNetCore.Server.IISIntegration { internal partial class IISHttpContext { + private const int HttpDataChunkStackLimit = 128; // 16 bytes per HTTP_DATA_CHUNK + /// /// Reads data from the Input pipe to the user. /// @@ -212,6 +214,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration var hr = 0; var nChunks = 0; + // Count the number of chunks in memory. if (buffer.IsSingleSegment) { nChunks = 1; @@ -224,8 +227,9 @@ namespace Microsoft.AspNetCore.Server.IISIntegration } } - if (buffer.IsSingleSegment) + if (nChunks == 1) { + // If there is only a single chunk, use fixed to get a pointer to the buffer var pDataChunks = stackalloc HttpApiTypes.HTTP_DATA_CHUNK[1]; fixed (byte* pBuffer = &MemoryMarshal.GetReference(buffer.First.Span)) @@ -238,35 +242,20 @@ namespace Microsoft.AspNetCore.Server.IISIntegration hr = NativeMethods.http_write_response_bytes(_pInProcessHandler, pDataChunks, nChunks, out fCompletionExpected); } } + else if (nChunks < HttpDataChunkStackLimit) + { + // To avoid stackoverflows, we will only stackalloc if the write size is less than the StackChunkLimit + // The stack size is IIS is by default 128/256 KB, so we are generous with this threshold. + var pDataChunks = stackalloc HttpApiTypes.HTTP_DATA_CHUNK[nChunks]; + hr = WriteSequenceToIIS(nChunks, buffer, pDataChunks, out fCompletionExpected); + } else { - // REVIEW: Do we need to guard against this getting too big? It seems unlikely that we'd have more than say 10 chunks in real life - var pDataChunks = stackalloc HttpApiTypes.HTTP_DATA_CHUNK[nChunks]; - var currentChunk = 0; - - // REVIEW: We don't really need this list since the memory is already pinned with the default pool, - // but shouldn't assume the pool implementation right now. Unfortunately, this causes a heap allocation... - var handles = new MemoryHandle[nChunks]; - - foreach (var b in buffer) + // Otherwise allocate the chunks on the heap. + var chunks = new HttpApiTypes.HTTP_DATA_CHUNK[nChunks]; + fixed (HttpApiTypes.HTTP_DATA_CHUNK* pDataChunks = chunks) { - ref var handle = ref handles[currentChunk]; - ref var chunk = ref pDataChunks[currentChunk]; - - handle = b.Retain(true); - - chunk.DataChunkType = HttpApiTypes.HTTP_DATA_CHUNK_TYPE.HttpDataChunkFromMemory; - chunk.fromMemory.BufferLength = (uint)b.Length; - chunk.fromMemory.pBuffer = (IntPtr)handle.Pointer; - - currentChunk++; - } - - hr = NativeMethods.http_write_response_bytes(_pInProcessHandler, pDataChunks, nChunks, out fCompletionExpected); - // Free the handles - foreach (var handle in handles) - { - handle.Dispose(); + hr = WriteSequenceToIIS(nChunks, buffer, pDataChunks, out fCompletionExpected); } } @@ -277,6 +266,39 @@ namespace Microsoft.AspNetCore.Server.IISIntegration return _operation; } + private unsafe int WriteSequenceToIIS(int nChunks, ReadOnlySequence buffer, HttpApiTypes.HTTP_DATA_CHUNK* pDataChunks, out bool fCompletionExpected) + { + var currentChunk = 0; + var hr = 0; + + // REVIEW: We don't really need this list since the memory is already pinned with the default pool, + // but shouldn't assume the pool implementation right now. Unfortunately, this causes a heap allocation... + var handles = new MemoryHandle[nChunks]; + + foreach (var b in buffer) + { + ref var handle = ref handles[currentChunk]; + ref var chunk = ref pDataChunks[currentChunk]; + handle = b.Retain(true); + + chunk.DataChunkType = HttpApiTypes.HTTP_DATA_CHUNK_TYPE.HttpDataChunkFromMemory; + chunk.fromMemory.BufferLength = (uint)b.Length; + chunk.fromMemory.pBuffer = (IntPtr)handle.Pointer; + + currentChunk++; + } + + hr = NativeMethods.http_write_response_bytes(_pInProcessHandler, pDataChunks, nChunks, out fCompletionExpected); + + // Free the handles + foreach (var handle in handles) + { + handle.Dispose(); + } + + return hr; + } + private unsafe IISAwaitable FlushToIISAsync() { // Calls flush diff --git a/test/IISIntegration.FunctionalTests/Inprocess/LargeResponseBodyTests.cs b/test/IISIntegration.FunctionalTests/Inprocess/LargeResponseBodyTests.cs index ac5c9e6db3..40db2cfdb8 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/LargeResponseBodyTests.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/LargeResponseBodyTests.cs @@ -18,11 +18,19 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests } [ConditionalTheory] + [InlineData(65000)] [InlineData(1000000)] [InlineData(10000000)] - public async Task LargeResponseBodyTestCheckAllResponseBodyBytesWritten(int query) + [InlineData(100000000)] + public async Task LargeResponseBodyTest_CheckAllResponseBodyBytesWritten(int query) { Assert.Equal(new string('a', query), await _fixture.Client.GetStringAsync($"/LargeResponseBody?length={query}")); } + + [ConditionalFact] + public async Task LargeResponseBodyFromFile_CheckAllResponseBodyBytesWritten() + { + Assert.Equal(200000000, (await _fixture.Client.GetStringAsync($"/LargeResponseFile")).Length); + } } } diff --git a/test/IISTestSite/Startup.cs b/test/IISTestSite/Startup.cs index 91f04a7aec..dd035b01a5 100644 --- a/test/IISTestSite/Startup.cs +++ b/test/IISTestSite/Startup.cs @@ -49,6 +49,7 @@ namespace IISTestSite app.Map("/TestInvalidReadOperations", TestInvalidReadOperations); app.Map("/TestInvalidWriteOperations", TestInvalidWriteOperations); app.Map("/TestReadOffsetWorks", TestReadOffsetWorks); + app.Map("/LargeResponseFile", LargeResponseFile); } private void ServerVariable(IApplicationBuilder app) @@ -628,5 +629,26 @@ namespace IISTestSite await context.Response.WriteAsync(success ? "Success" : "Failure"); }); } + + private void LargeResponseFile(IApplicationBuilder app) + { + app.Run(async ctx => + { + var tempFile = Path.GetTempFileName(); + File.WriteAllText(tempFile, new string('a', 200000000)); + await ctx.Response.SendFileAsync(tempFile, 0, null); + + // Try to delete the file from the temp directory. If it fails, don't report an error + // to the application. File should eventually be cleaned up from the temp directory + // by OS. + try + { + File.Delete(tempFile); + } + catch (Exception) + { + } + }); + } } } From 2bb9a8aaf950d27def213b800b0712b040293161 Mon Sep 17 00:00:00 2001 From: pan-wang Date: Mon, 19 Mar 2018 15:30:55 -0700 Subject: [PATCH 3/3] Adding handlerSettings configuration to aspnetcore schema for future use (#689) --- src/AspNetCore/aspnetcore_schema.xml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/AspNetCore/aspnetcore_schema.xml b/src/AspNetCore/aspnetcore_schema.xml index d4db49bfb9..d65be07195 100644 --- a/src/AspNetCore/aspnetcore_schema.xml +++ b/src/AspNetCore/aspnetcore_schema.xml @@ -27,7 +27,6 @@ - @@ -38,5 +37,11 @@ + + + + + +