From 462f9c6dfd7e07fd4f87f2d884cc5c20af5a5e42 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 12 May 2019 21:07:13 +1200 Subject: [PATCH 1/5] Improve TestServer support for Response.StartAsync --- .../TestHost/src/HttpContextBuilder.cs | 8 ++-- src/Hosting/TestHost/src/ResponseFeature.cs | 8 +++- .../TestHost/test/ClientHandlerTests.cs | 38 +++++++++++++++++++ .../TestHost/test/ResponseFeatureTests.cs | 18 ++++++++- 4 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/Hosting/TestHost/src/HttpContextBuilder.cs b/src/Hosting/TestHost/src/HttpContextBuilder.cs index 710ffa3d8612..08c79d4c6c95 100644 --- a/src/Hosting/TestHost/src/HttpContextBuilder.cs +++ b/src/Hosting/TestHost/src/HttpContextBuilder.cs @@ -24,6 +24,7 @@ internal class HttpContextBuilder : IHttpBodyControlFeature private readonly RequestLifetimeFeature _requestLifetimeFeature = new RequestLifetimeFeature(); private readonly ResponseTrailersFeature _responseTrailersFeature = new ResponseTrailersFeature(); private bool _pipelineFinished; + private bool _returningResponse; private Context _testContext; private Action _responseReadCompleteCallback; @@ -132,12 +133,13 @@ internal async Task CompleteResponseAsync() internal async Task ReturnResponseMessageAsync() { - // Check if the response has already started because the TrySetResult below could happen a bit late + // Check if the response is already returning because the TrySetResult below could happen a bit late // (as it happens on a different thread) by which point the CompleteResponseAsync could run and calls this // method again. - if (!_responseFeature.HasStarted) + if (!_returningResponse) { - // Sets HasStarted + _returningResponse = true; + try { await _responseFeature.FireOnSendingHeadersAsync(); diff --git a/src/Hosting/TestHost/src/ResponseFeature.cs b/src/Hosting/TestHost/src/ResponseFeature.cs index c6c7b47e18dd..5deea18f88c3 100644 --- a/src/Hosting/TestHost/src/ResponseFeature.cs +++ b/src/Hosting/TestHost/src/ResponseFeature.cs @@ -3,13 +3,14 @@ using System; using System.IO; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; namespace Microsoft.AspNetCore.TestHost { - internal class ResponseFeature : IHttpResponseFeature + internal class ResponseFeature : IHttpResponseFeature, IHttpResponseStartFeature { private Func _responseStartingAsync = () => Task.FromResult(true); private Func _responseCompletedAsync = () => Task.FromResult(true); @@ -107,5 +108,10 @@ public Task FireOnResponseCompletedAsync() { return _responseCompletedAsync(); } + + public Task StartAsync(CancellationToken token = default) + { + return FireOnSendingHeadersAsync(); + } } } diff --git a/src/Hosting/TestHost/test/ClientHandlerTests.cs b/src/Hosting/TestHost/test/ClientHandlerTests.cs index 73d37c0159b9..02360987d8d3 100644 --- a/src/Hosting/TestHost/test/ClientHandlerTests.cs +++ b/src/Hosting/TestHost/test/ClientHandlerTests.cs @@ -153,6 +153,44 @@ public async Task ServerTrailersSetOnResponseAfterContentRead() }); } + [Fact] + public async Task ResponseStartAsync() + { + var hasStartedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var hasAssertedResponseTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + bool? preHasStarted = null; + bool? postHasStarted = null; + var handler = new ClientHandler(PathString.Empty, new DummyApplication(async context => + { + preHasStarted = context.Response.HasStarted; + + await context.Response.StartAsync(); + + postHasStarted = context.Response.HasStarted; + + hasStartedTcs.TrySetResult(null); + + await hasAssertedResponseTcs.Task; + })); + + var invoker = new HttpMessageInvoker(handler); + var message = new HttpRequestMessage(HttpMethod.Post, "https://example.com/"); + + var responseTask = invoker.SendAsync(message, CancellationToken.None); + + await hasStartedTcs.Task; + + Assert.False(responseTask.IsCompleted, "HttpResponse.StartAsync does not return response"); + + hasAssertedResponseTcs.TrySetResult(null); + + await responseTask; + + Assert.False(preHasStarted); + Assert.True(postHasStarted); + } + [Fact] public async Task ResubmitRequestWorks() { diff --git a/src/Hosting/TestHost/test/ResponseFeatureTests.cs b/src/Hosting/TestHost/test/ResponseFeatureTests.cs index f8af4cf64d24..17ac7fc40835 100644 --- a/src/Hosting/TestHost/test/ResponseFeatureTests.cs +++ b/src/Hosting/TestHost/test/ResponseFeatureTests.cs @@ -25,6 +25,22 @@ public async Task StatusCode_DefaultsTo200() Assert.True(responseInformation.Headers.IsReadOnly); } + [Fact] + public async Task StartAsync_StartsResponse() + { + // Arrange & Act + var responseInformation = new ResponseFeature(); + + // Assert + Assert.Equal(200, responseInformation.StatusCode); + Assert.False(responseInformation.HasStarted); + + await responseInformation.StartAsync(); + + Assert.True(responseInformation.HasStarted); + Assert.True(responseInformation.Headers.IsReadOnly); + } + [Fact] public void OnStarting_ThrowsWhenHasStarted() { @@ -65,4 +81,4 @@ public void StatusCode_MustBeGreaterThan99() responseInformation.StatusCode = 1000; } } -} \ No newline at end of file +} From 3713db8b2746bc92a80cabc95699c9fcc657ac76 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 12 May 2019 22:25:15 +1200 Subject: [PATCH 2/5] Actually work --- src/Hosting/TestHost/src/HttpContextBuilder.cs | 1 + src/Hosting/TestHost/test/ClientHandlerTests.cs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/Hosting/TestHost/src/HttpContextBuilder.cs b/src/Hosting/TestHost/src/HttpContextBuilder.cs index 08c79d4c6c95..fb90cd841ec1 100644 --- a/src/Hosting/TestHost/src/HttpContextBuilder.cs +++ b/src/Hosting/TestHost/src/HttpContextBuilder.cs @@ -41,6 +41,7 @@ internal HttpContextBuilder(IHttpApplication application, bool allowSyn _httpContext.Features.Set(this); _httpContext.Features.Set(_responseFeature); + _httpContext.Features.Set(_responseFeature); _httpContext.Features.Set(_requestLifetimeFeature); _httpContext.Features.Set(_responseTrailersFeature); diff --git a/src/Hosting/TestHost/test/ClientHandlerTests.cs b/src/Hosting/TestHost/test/ClientHandlerTests.cs index 02360987d8d3..75399284602c 100644 --- a/src/Hosting/TestHost/test/ClientHandlerTests.cs +++ b/src/Hosting/TestHost/test/ClientHandlerTests.cs @@ -179,10 +179,14 @@ public async Task ResponseStartAsync() var responseTask = invoker.SendAsync(message, CancellationToken.None); + // Ensure StartAsync has been called in response await hasStartedTcs.Task; + // Delay so async thread would have had time to attempt to return response + await Task.Delay(100); Assert.False(responseTask.IsCompleted, "HttpResponse.StartAsync does not return response"); + // Asserted that response return was checked, allow response to finish hasAssertedResponseTcs.TrySetResult(null); await responseTask; From 8d9eb831599b0e5835a9bd26f28e3daf7374984a Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 12 May 2019 23:43:04 +1200 Subject: [PATCH 3/5] Fix tests --- src/Hosting/TestHost/src/ResponseFeature.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/Hosting/TestHost/src/ResponseFeature.cs b/src/Hosting/TestHost/src/ResponseFeature.cs index 5deea18f88c3..075df712ced2 100644 --- a/src/Hosting/TestHost/src/ResponseFeature.cs +++ b/src/Hosting/TestHost/src/ResponseFeature.cs @@ -99,9 +99,12 @@ public void OnCompleted(Func callback, object state) public async Task FireOnSendingHeadersAsync() { - await _responseStartingAsync(); - HasStarted = true; - _headers.IsReadOnly = true; + if (!HasStarted) + { + await _responseStartingAsync(); + HasStarted = true; + _headers.IsReadOnly = true; + } } public Task FireOnResponseCompletedAsync() From 8f0338b210a09722401025d02c870c56b2aabd81 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Mon, 13 May 2019 09:49:19 +1200 Subject: [PATCH 4/5] Ensure HasStarted set to true --- .../TestHost/src/HttpContextBuilder.cs | 3 +- src/Hosting/TestHost/src/ResponseFeature.cs | 31 ++++++++++++++----- .../TestHost/test/ResponseFeatureTests.cs | 15 ++++++--- .../test/UnitTests/ExceptionHandlerTest.cs | 3 +- 4 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/Hosting/TestHost/src/HttpContextBuilder.cs b/src/Hosting/TestHost/src/HttpContextBuilder.cs index fb90cd841ec1..287e4aacf7b2 100644 --- a/src/Hosting/TestHost/src/HttpContextBuilder.cs +++ b/src/Hosting/TestHost/src/HttpContextBuilder.cs @@ -20,7 +20,7 @@ internal class HttpContextBuilder : IHttpBodyControlFeature private readonly TaskCompletionSource _responseTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); private readonly ResponseStream _responseStream; - private readonly ResponseFeature _responseFeature = new ResponseFeature(); + private readonly ResponseFeature _responseFeature; private readonly RequestLifetimeFeature _requestLifetimeFeature = new RequestLifetimeFeature(); private readonly ResponseTrailersFeature _responseTrailersFeature = new ResponseTrailersFeature(); private bool _pipelineFinished; @@ -34,6 +34,7 @@ internal HttpContextBuilder(IHttpApplication application, bool allowSyn AllowSynchronousIO = allowSynchronousIO; _preserveExecutionContext = preserveExecutionContext; _httpContext = new DefaultHttpContext(); + _responseFeature = new ResponseFeature(Abort); var request = _httpContext.Request; request.Protocol = "HTTP/1.1"; diff --git a/src/Hosting/TestHost/src/ResponseFeature.cs b/src/Hosting/TestHost/src/ResponseFeature.cs index 075df712ced2..2af0e3de365b 100644 --- a/src/Hosting/TestHost/src/ResponseFeature.cs +++ b/src/Hosting/TestHost/src/ResponseFeature.cs @@ -12,13 +12,15 @@ namespace Microsoft.AspNetCore.TestHost { internal class ResponseFeature : IHttpResponseFeature, IHttpResponseStartFeature { + private readonly HeaderDictionary _headers = new HeaderDictionary(); + private readonly Action _abort; + private Func _responseStartingAsync = () => Task.FromResult(true); private Func _responseCompletedAsync = () => Task.FromResult(true); - private HeaderDictionary _headers = new HeaderDictionary(); private int _statusCode; private string _reasonPhrase; - public ResponseFeature() + public ResponseFeature(Action abort) { Headers = _headers; Body = new MemoryStream(); @@ -26,6 +28,7 @@ public ResponseFeature() // 200 is the default status code all the way down to the host, so we set it // here to be consistent with the rest of the hosts when writing tests. StatusCode = 200; + _abort = abort; } public int StatusCode @@ -101,9 +104,15 @@ public async Task FireOnSendingHeadersAsync() { if (!HasStarted) { - await _responseStartingAsync(); - HasStarted = true; - _headers.IsReadOnly = true; + try + { + await _responseStartingAsync(); + } + finally + { + HasStarted = true; + _headers.IsReadOnly = true; + } } } @@ -112,9 +121,17 @@ public Task FireOnResponseCompletedAsync() return _responseCompletedAsync(); } - public Task StartAsync(CancellationToken token = default) + public async Task StartAsync(CancellationToken token = default) { - return FireOnSendingHeadersAsync(); + try + { + await FireOnSendingHeadersAsync(); + } + catch (Exception ex) + { + _abort(ex); + throw; + } } } } diff --git a/src/Hosting/TestHost/test/ResponseFeatureTests.cs b/src/Hosting/TestHost/test/ResponseFeatureTests.cs index 17ac7fc40835..cea2f6121485 100644 --- a/src/Hosting/TestHost/test/ResponseFeatureTests.cs +++ b/src/Hosting/TestHost/test/ResponseFeatureTests.cs @@ -13,7 +13,7 @@ public class ResponseFeatureTests public async Task StatusCode_DefaultsTo200() { // Arrange & Act - var responseInformation = new ResponseFeature(); + var responseInformation = CreateResponseFeature(); // Assert Assert.Equal(200, responseInformation.StatusCode); @@ -29,7 +29,7 @@ public async Task StatusCode_DefaultsTo200() public async Task StartAsync_StartsResponse() { // Arrange & Act - var responseInformation = new ResponseFeature(); + var responseInformation = CreateResponseFeature(); // Assert Assert.Equal(200, responseInformation.StatusCode); @@ -45,7 +45,7 @@ public async Task StartAsync_StartsResponse() public void OnStarting_ThrowsWhenHasStarted() { // Arrange - var responseInformation = new ResponseFeature(); + var responseInformation = CreateResponseFeature(); responseInformation.HasStarted = true; // Act & Assert @@ -61,7 +61,7 @@ public void OnStarting_ThrowsWhenHasStarted() [Fact] public void StatusCode_ThrowsWhenHasStarted() { - var responseInformation = new ResponseFeature(); + var responseInformation = CreateResponseFeature(); responseInformation.HasStarted = true; Assert.Throws(() => responseInformation.StatusCode = 400); @@ -71,7 +71,7 @@ public void StatusCode_ThrowsWhenHasStarted() [Fact] public void StatusCode_MustBeGreaterThan99() { - var responseInformation = new ResponseFeature(); + var responseInformation = CreateResponseFeature(); Assert.Throws(() => responseInformation.StatusCode = 99); Assert.Throws(() => responseInformation.StatusCode = 0); @@ -80,5 +80,10 @@ public void StatusCode_MustBeGreaterThan99() responseInformation.StatusCode = 200; responseInformation.StatusCode = 1000; } + + private ResponseFeature CreateResponseFeature() + { + return new ResponseFeature(ex => { }); + } } } diff --git a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs index 0ee748add761..f36bd33d784b 100644 --- a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs +++ b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs @@ -7,6 +7,7 @@ using System.IO; using System.Linq; using System.Net; +using System.Text; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; @@ -149,7 +150,7 @@ public async Task ClearsResponseBuffer_BeforeRequestIsReexecuted() app.Run(async (context) => { // Write some content into the response before throwing exception - await context.Response.WriteAsync(new string('a', 100)); + await context.Response.Body.WriteAsync(Encoding.UTF8.GetBytes(new string('a', 100))); throw new InvalidOperationException("Invalid input provided."); }); From f190f929ad4664f67043bcb0716cd77c3b562d7a Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Mon, 13 May 2019 18:05:27 +1200 Subject: [PATCH 5/5] PR feedback --- .../Diagnostics/test/UnitTests/ExceptionHandlerTest.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs index f36bd33d784b..2ec721730ce7 100644 --- a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs +++ b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs @@ -13,6 +13,7 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; using Xunit; @@ -115,6 +116,8 @@ public async Task ClearsResponseBuffer_BeforeRequestIsReexecuted() // add response buffering app.Use(async (httpContext, next) => { + httpContext.Features.Set(null); + var response = httpContext.Response; var originalResponseBody = response.Body; var bufferingStream = new MemoryStream(); @@ -150,7 +153,7 @@ public async Task ClearsResponseBuffer_BeforeRequestIsReexecuted() app.Run(async (context) => { // Write some content into the response before throwing exception - await context.Response.Body.WriteAsync(Encoding.UTF8.GetBytes(new string('a', 100))); + await context.Response.WriteAsync(new string('a', 100)); throw new InvalidOperationException("Invalid input provided."); });