From 18391dd2e427dbb8083dd3d9a7cdac099c20e817 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Mon, 12 Aug 2019 13:11:21 -0700 Subject: [PATCH 01/14] Revert "Clear route values after middleware invocation." This reverts commit c54a7209d7bc41e163ca8767e59765fbc37d27ce. --- .../Routing/src/EndpointRoutingMiddleware.cs | 5 +- ...RoutingApplicationBuilderExtensionsTest.cs | 3 +- .../EndpointRoutingMiddlewareTest.cs | 86 ++++++------------- 3 files changed, 29 insertions(+), 65 deletions(-) diff --git a/src/Http/Routing/src/EndpointRoutingMiddleware.cs b/src/Http/Routing/src/EndpointRoutingMiddleware.cs index 4ae20514d2..65596ecfd4 100644 --- a/src/Http/Routing/src/EndpointRoutingMiddleware.cs +++ b/src/Http/Routing/src/EndpointRoutingMiddleware.cs @@ -3,10 +3,10 @@ using System; using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Routing.Matching; using Microsoft.Extensions.Logging; @@ -116,9 +116,6 @@ namespace Microsoft.AspNetCore.Routing { // This allows a second call in a single request (such as from the ErrorHandlerMiddleware) to perform routing again. httpContext.SetEndpoint(endpoint: null); - - var routeValuesFeature = httpContext.Features.Get(); - routeValuesFeature?.RouteValues?.Clear(); } } diff --git a/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs index e1451329ea..7f51f6e9a7 100644 --- a/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs @@ -74,7 +74,7 @@ namespace Microsoft.AspNetCore.Builder } [Fact] - public async Task UseRouting_ServicesRegistered_Match_DoesNotSetFeature() + public async Task UseRouting_ServicesRegistered_Match_DoesNotSetsFeature() { // Arrange var endpoint = new RouteEndpoint( @@ -104,6 +104,7 @@ namespace Microsoft.AspNetCore.Builder // Assert var feature = httpContext.Features.Get(); Assert.NotNull(feature); + Assert.Same(endpoint, httpContext.GetEndpoint()); } [Fact] diff --git a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs index 8f97bfd5d2..c3cf38cc32 100644 --- a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs +++ b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs @@ -102,26 +102,6 @@ namespace Microsoft.AspNetCore.Routing Assert.Null(endpointFeature.Endpoint); } - [Fact] - public async Task Invoke_OnCall_SetsEndpointFeatureAndResetsRouteValues() - { - // Arrange - var httpContext = CreateHttpContext(); - var initialRouteData = new RouteData(); - initialRouteData.Values["test"] = true; - httpContext.Features.Set(new RoutingFeature() - { - RouteData = initialRouteData, - }); - var middleware = CreateMiddleware(); - - // Act - await middleware.Invoke(httpContext); - - // Assert - Assert.Null(httpContext.GetRouteValue("test")); - } - [Fact] public async Task Invoke_SkipsRoutingAndMaintainsEndpoint_IfEndpointSet() { @@ -182,29 +162,22 @@ namespace Microsoft.AspNetCore.Routing { // Arrange var httpContext = CreateHttpContext(); - var nextCalled = false; - var middleware = CreateMiddleware(next: context => - { - var routeData = httpContext.GetRouteData(); - var routeValue = httpContext.GetRouteValue("controller"); - var routeValuesFeature = httpContext.Features.Get(); - nextCalled = true; + var middleware = CreateMiddleware(); - // Assert - Assert.NotNull(routeData); - Assert.Equal("Home", (string)routeValue); - - // changing route data value is reflected in endpoint feature values - routeData.Values["testKey"] = "testValue"; - Assert.Equal("testValue", routeValuesFeature.RouteValues["testKey"]); - - return Task.CompletedTask; - }); - - // Act & Assert + // Act await middleware.Invoke(httpContext); - Assert.True(nextCalled); + var routeData = httpContext.GetRouteData(); + var routeValue = httpContext.GetRouteValue("controller"); + var routeValuesFeature = httpContext.Features.Get(); + + // Assert + Assert.NotNull(routeData); + Assert.Equal("Home", (string)routeValue); + + // changing route data value is reflected in endpoint feature values + routeData.Values["testKey"] = "testValue"; + Assert.Equal("testValue", routeValuesFeature.RouteValues["testKey"]); } [Fact] @@ -212,29 +185,22 @@ namespace Microsoft.AspNetCore.Routing { // Arrange var httpContext = CreateHttpContext(); - var called = false; - var middleware = CreateMiddleware(next: context => - { - var routeData = httpContext.GetRouteData(); - var routeValue = httpContext.GetRouteValue("controller"); - var routeValuesFeature = httpContext.Features.Get(); - called = true; + var middleware = CreateMiddleware(); - // Assert - Assert.NotNull(routeData); - Assert.Equal("Home", (string)routeValue); - - // changing route data value is reflected in endpoint feature values - routeData.Values["testKey"] = "testValue"; - Assert.Equal("testValue", routeValuesFeature.RouteValues["testKey"]); - - return Task.CompletedTask; - }); - - // Act & Assert + // Act await middleware.Invoke(httpContext); - Assert.True(called); + var routeData = httpContext.GetRouteData(); + var routeValue = httpContext.GetRouteValue("controller"); + var routeValuesFeature = httpContext.Features.Get(); + + // Assert + Assert.NotNull(routeData); + Assert.Equal("Home", (string)routeValue); + + // changing route data value is reflected in endpoint feature values + routeData.Values["testKey"] = "testValue"; + Assert.Equal("testValue", routeValuesFeature.RouteValues["testKey"]); } [Fact] From 44431151c74a4da3f926223376de6934a6afefbd Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Mon, 12 Aug 2019 13:13:05 -0700 Subject: [PATCH 02/14] Revert "Addressed code review comments." This reverts commit 479d5ed40ada72785831259a6e5a6112635c7a32. --- .../Routing/src/EndpointRoutingMiddleware.cs | 6 ++- .../EndpointRoutingMiddlewareTest.cs | 42 ------------------- 2 files changed, 4 insertions(+), 44 deletions(-) diff --git a/src/Http/Routing/src/EndpointRoutingMiddleware.cs b/src/Http/Routing/src/EndpointRoutingMiddleware.cs index 65596ecfd4..cfaff0befb 100644 --- a/src/Http/Routing/src/EndpointRoutingMiddleware.cs +++ b/src/Http/Routing/src/EndpointRoutingMiddleware.cs @@ -52,7 +52,7 @@ namespace Microsoft.AspNetCore.Routing { Log.MatchSkipped(_logger, endpoint); - // Someone else set the endpoint, we'll let them handle the clearing of the endpoint. + // Someone else set the endpoint, we'll let them handle the unsetting. return _next(httpContext); } @@ -88,6 +88,7 @@ namespace Microsoft.AspNetCore.Routing } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private async Task SetRoutingAndContinue(HttpContext httpContext) { // If there was no mutation of the endpoint then log failure @@ -114,7 +115,8 @@ namespace Microsoft.AspNetCore.Routing } finally { - // This allows a second call in a single request (such as from the ErrorHandlerMiddleware) to perform routing again. + // We unset the endpoint after calling through to the next middleware. This enables any future calls into + // endpoint routing don't no-op from there already being an endpoint set. httpContext.SetEndpoint(endpoint: null); } } diff --git a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs index c3cf38cc32..b2e08d9a2c 100644 --- a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs +++ b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs @@ -20,48 +20,6 @@ namespace Microsoft.AspNetCore.Routing { public class EndpointRoutingMiddlewareTest { - [Fact] - public async Task Invoke_ChangedPath_ResultsInDifferentResult() - { - // Arrange - var httpContext = CreateHttpContext(); - var matcher = new Mock(); - var pathToEndpoints = new Dictionary() - { - ["/initial"] = new Endpoint(c => Task.CompletedTask, new EndpointMetadataCollection(), "initialEndpoint"), - ["/changed"] = new Endpoint(c => Task.CompletedTask, new EndpointMetadataCollection(), "changedEndpoint") - }; - matcher.Setup(m => m.MatchAsync(httpContext)) - .Callback(context => - { - var endpointToSet = pathToEndpoints[context.Request.Path]; - context.SetEndpoint(endpointToSet); - }) - .Returns(Task.CompletedTask) - .Verifiable(); - var matcherFactory = Mock.Of(factory => factory.CreateMatcher(It.IsAny()) == matcher.Object); - var middleware = CreateMiddleware( - matcherFactory: matcherFactory, - next: context => - { - Assert.True(pathToEndpoints.TryGetValue(context.Request.Path, out var expectedEndpoint)); - - var currentEndpoint = context.GetEndpoint(); - Assert.Equal(expectedEndpoint, currentEndpoint); - - return Task.CompletedTask; - }); - - // Act - httpContext.Request.Path = "/initial"; - await middleware.Invoke(httpContext); - httpContext.Request.Path = "/changed"; - await middleware.Invoke(httpContext); - - // Assert - matcher.Verify(); - } - [Fact] public async Task Invoke_OnException_ResetsEndpoint() { From 4f6022323baa7d68efd19df93c4d0c4aef6f5511 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Mon, 12 Aug 2019 13:13:13 -0700 Subject: [PATCH 03/14] Revert "Fix endpoint routing statefulness." This reverts commit 9a6881b0e8d2697e5a6a12dd9ddbdfaf6ce6437f. --- .../Routing/src/EndpointRoutingMiddleware.cs | 15 ++------- .../EndpointRoutingMiddlewareTest.cs | 33 +++---------------- 2 files changed, 6 insertions(+), 42 deletions(-) diff --git a/src/Http/Routing/src/EndpointRoutingMiddleware.cs b/src/Http/Routing/src/EndpointRoutingMiddleware.cs index cfaff0befb..482e86bb6d 100644 --- a/src/Http/Routing/src/EndpointRoutingMiddleware.cs +++ b/src/Http/Routing/src/EndpointRoutingMiddleware.cs @@ -51,8 +51,6 @@ namespace Microsoft.AspNetCore.Routing if (endpoint != null) { Log.MatchSkipped(_logger, endpoint); - - // Someone else set the endpoint, we'll let them handle the unsetting. return _next(httpContext); } @@ -89,7 +87,7 @@ namespace Microsoft.AspNetCore.Routing } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private async Task SetRoutingAndContinue(HttpContext httpContext) + private Task SetRoutingAndContinue(HttpContext httpContext) { // If there was no mutation of the endpoint then log failure var endpoint = httpContext.GetEndpoint(); @@ -109,16 +107,7 @@ namespace Microsoft.AspNetCore.Routing Log.MatchSuccess(_logger, endpoint); } - try - { - await _next(httpContext); - } - finally - { - // We unset the endpoint after calling through to the next middleware. This enables any future calls into - // endpoint routing don't no-op from there already being an endpoint set. - httpContext.SetEndpoint(endpoint: null); - } + return _next(httpContext); } // Initialization is async to avoid blocking threads while reflection and things diff --git a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs index b2e08d9a2c..48e1157195 100644 --- a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs +++ b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs @@ -21,30 +21,7 @@ namespace Microsoft.AspNetCore.Routing public class EndpointRoutingMiddlewareTest { [Fact] - public async Task Invoke_OnException_ResetsEndpoint() - { - // Arrange - var httpContext = CreateHttpContext(); - - var middleware = CreateMiddleware(next: context => throw new Exception()); - - // Act - try - { - await middleware.Invoke(httpContext); - } - catch - { - // Do nothing, we expect the test to throw. - } - - // Assert - var endpoint = httpContext.GetEndpoint(); - Assert.Null(endpoint); - } - - [Fact] - public async Task Invoke_OnCall_SetsEndpointFeatureAndResetsEndpoint() + public async Task Invoke_OnCall_SetsEndpointFeature() { // Arrange var httpContext = CreateHttpContext(); @@ -57,16 +34,14 @@ namespace Microsoft.AspNetCore.Routing // Assert var endpointFeature = httpContext.Features.Get(); Assert.NotNull(endpointFeature); - Assert.Null(endpointFeature.Endpoint); } [Fact] - public async Task Invoke_SkipsRoutingAndMaintainsEndpoint_IfEndpointSet() + public async Task Invoke_SkipsRouting_IfEndpointSet() { // Arrange var httpContext = CreateHttpContext(); - var expectedEndpoint = new Endpoint(c => Task.CompletedTask, new EndpointMetadataCollection(), "myapp"); - httpContext.SetEndpoint(expectedEndpoint); + httpContext.SetEndpoint(new Endpoint(c => Task.CompletedTask, new EndpointMetadataCollection(), "myapp")); var middleware = CreateMiddleware(); @@ -75,7 +50,7 @@ namespace Microsoft.AspNetCore.Routing // Assert var endpoint = httpContext.GetEndpoint(); - Assert.Same(expectedEndpoint, endpoint); + Assert.NotNull(endpoint); Assert.Equal("myapp", endpoint.DisplayName); } From 1f0641f5c0c8552f9605ff782ba1a27c06f4cedf Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Mon, 12 Aug 2019 13:41:36 -0700 Subject: [PATCH 04/14] Reset endpoint and route values during exception handling. - We initially did this change as part of EndpointRouting but the impact of that change resulted in a variety of performance regressions. To mitigate the impact of resetting state for a request we now only reset the state when an exception has occurred in a way that does not require any additional state machines to be allocated. - Added a test to validate that http context state gets reset on exception handling. #12897 --- .../ExceptionHandlerMiddleware.cs | 15 +++- .../ExceptionHandlerMiddlewareTest.cs | 87 +++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs diff --git a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs index 112685a9e5..5d280b5f43 100644 --- a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs +++ b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs @@ -7,6 +7,7 @@ using System.Runtime.ExceptionServices; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Microsoft.Net.Http.Headers; @@ -103,7 +104,8 @@ namespace Microsoft.AspNetCore.Diagnostics } try { - context.Response.Clear(); + ClearHttpContext(context); + var exceptionHandlerFeature = new ExceptionHandlerFeature() { Error = edi.SourceException, @@ -137,6 +139,17 @@ namespace Microsoft.AspNetCore.Diagnostics edi.Throw(); // Re-throw the original if we couldn't handle it } + private static void ClearHttpContext(HttpContext context) + { + context.Response.Clear(); + + // An endpoint may have already been set. Since we're going to re-invoke the middleware pipeline we need to reset + // the endpoint and route values to ensure things are re-calculated. + context.SetEndpoint(endpoint: null); + var routeValuesFeature = context.Features.Get(); + routeValuesFeature?.RouteValues?.Clear(); + } + private static Task ClearCacheHeaders(object state) { var headers = ((HttpResponse)state).Headers; diff --git a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs new file mode 100644 index 0000000000..6d08be5d3f --- /dev/null +++ b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs @@ -0,0 +1,87 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Diagnostics; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Moq; +using Xunit; + +namespace Microsoft.AspNetCore.Diagnostics +{ + public class ExceptionHandlerMiddlewareTest + { + [Fact] + public async Task Invoke_ExceptionThrownResultsInClearedRouteValuesAndEndpoint() + { + // Arrange + var httpContext = CreateHttpContext(); + httpContext.SetEndpoint(new Endpoint((_) => Task.CompletedTask, new EndpointMetadataCollection(), "Test")); + httpContext.Request.RouteValues["John"] = "Doe"; + + var optionsAccessor = CreateOptionsAccessor( + exceptionHandler: context => + { + Assert.Empty(context.Request.RouteValues); + Assert.Null(context.GetEndpoint()); + return Task.CompletedTask; + }); + var middleware = CreateMiddleware(_ => throw new InvalidOperationException(), optionsAccessor); + + // Act & Assert + await middleware.Invoke(httpContext); + } + + private HttpContext CreateHttpContext() + { + var httpContext = new DefaultHttpContext + { + RequestServices = new TestServiceProvider() + }; + + return httpContext; + } + + private IOptions CreateOptionsAccessor( + RequestDelegate exceptionHandler = null, + string exceptionHandlingPath = null) + { + exceptionHandler ??= c => Task.CompletedTask; + var options = new ExceptionHandlerOptions() + { + ExceptionHandler = exceptionHandler, + ExceptionHandlingPath = exceptionHandlingPath, + }; + var optionsAccessor = Mock.Of>(o => o.Value == options); + return optionsAccessor; + } + + private ExceptionHandlerMiddleware CreateMiddleware( + RequestDelegate next, + IOptions options) + { + next ??= c => Task.CompletedTask; + var listener = new DiagnosticListener("Microsoft.AspNetCore"); + + var middleware = new ExceptionHandlerMiddleware( + next, + NullLoggerFactory.Instance, + options, + listener); + + return middleware; + } + + private class TestServiceProvider : IServiceProvider + { + public object GetService(Type serviceType) + { + throw new NotImplementedException(); + } + } + } +} From aa34cf3212855ee6041e1ee1a0ca0d2cf4cf93e7 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Tue, 13 Aug 2019 13:42:20 -0700 Subject: [PATCH 05/14] Update debian arm64 Helix queue image (#13009) Update debian arm64 Helix queue image --- docs/Helix.md | 2 +- eng/targets/Helix.Common.props | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/Helix.md b/docs/Helix.md index c685977981..e8261acc85 100644 --- a/docs/Helix.md +++ b/docs/Helix.md @@ -22,7 +22,7 @@ To run tests for the entire repo, run: .\eng\scripts\TestHelix.ps1 ``` -This will restore, and then publish all of the test projects including some bootstrapping scripts that will install the correct dotnet runtime/sdk before running the test assemblies on the helix machine, and upload the job to helix, it won't wait for the jobs to complete, but you can go to https://mc.dot.net/#/user/$(your user name)/builds. +This will restore, and then publish all of the test projects including some bootstrapping scripts that will install the correct dotnet runtime/sdk before running the test assemblies on the helix machine, and upload the job to helix. ## How do I look at the results of a helix run on Azure Pipelines? diff --git a/eng/targets/Helix.Common.props b/eng/targets/Helix.Common.props index a8e3e70ac4..8ea26294a2 100644 --- a/eng/targets/Helix.Common.props +++ b/eng/targets/Helix.Common.props @@ -11,7 +11,7 @@ - + @@ -27,11 +27,11 @@ - + - - + +