From 9c64880390fe987769cd2a7fe823344594e2483b Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 16 Nov 2018 05:43:04 +0000 Subject: [PATCH 1/4] Use object indirection in HttpContextAccessor --- .../HttpContextAccessor.cs | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs index 897c27f7..cd33cfa8 100644 --- a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs +++ b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs @@ -7,21 +7,39 @@ namespace Microsoft.AspNetCore.Http { public class HttpContextAccessor : IHttpContextAccessor { - private static AsyncLocal<(string traceIdentifier, HttpContext context)> _httpContextCurrent = new AsyncLocal<(string traceIdentifier, HttpContext context)>(); + private static AsyncLocal _httpContextCurrent = new AsyncLocal(); public HttpContext HttpContext { get { - var value = _httpContextCurrent.Value; - // Only return the context if the stored request id matches the stored trace identifier - // context.TraceIdentifier is cleared by HttpContextFactory.Dispose. - return value.traceIdentifier == value.context?.TraceIdentifier ? value.context : null; + return _httpContextCurrent.Value?.Context; } set { - _httpContextCurrent.Value = (value?.TraceIdentifier, value); + var holder = _httpContextCurrent.Value; + if (holder != null) + { + // Kill current HttpContext trapped in the AsyncLocals, as its done. + holder.Context = null; + } + + if (value != null) + { + // Use an object indirection to hold the HttpContext in the AsyncLocal, + // so it can be cleared in all ExecutionContexts when its cleared. + _httpContextCurrent.Value = new HttpContextHolder { Context = value }; + } + else + { + _httpContextCurrent.Value = null; + } } } + + private class HttpContextHolder + { + public HttpContext Context; + } } } From 64a02041d86f38cd653c216494139d8447a14617 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 16 Nov 2018 06:05:07 +0000 Subject: [PATCH 2/4] Change tests --- src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs | 4 ++-- src/Microsoft.AspNetCore.Http/HttpContextFactory.cs | 4 ---- .../HttpContextAccessorTests.cs | 11 +---------- .../HttpContextFactoryTests.cs | 2 -- 4 files changed, 3 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs index cd33cfa8..08b4d6f9 100644 --- a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs +++ b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs @@ -20,7 +20,7 @@ public HttpContext HttpContext var holder = _httpContextCurrent.Value; if (holder != null) { - // Kill current HttpContext trapped in the AsyncLocals, as its done. + // Clear current HttpContext trapped in the AsyncLocals, as its done. holder.Context = null; } @@ -30,7 +30,7 @@ public HttpContext HttpContext // so it can be cleared in all ExecutionContexts when its cleared. _httpContextCurrent.Value = new HttpContextHolder { Context = value }; } - else + else if (holder != null) { _httpContextCurrent.Value = null; } diff --git a/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs b/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs index f293ef47..8236a388 100644 --- a/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs +++ b/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs @@ -53,10 +53,6 @@ public void Dispose(HttpContext httpContext) { _httpContextAccessor.HttpContext = null; } - - // Null out the TraceIdentifier here as a sign that this request is done, - // the HttpContextAccessor implementation relies on this to detect that the request is over - httpContext.TraceIdentifier = null; } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Http.Tests/HttpContextAccessorTests.cs b/test/Microsoft.AspNetCore.Http.Tests/HttpContextAccessorTests.cs index c1521b1b..c224a66a 100644 --- a/test/Microsoft.AspNetCore.Http.Tests/HttpContextAccessorTests.cs +++ b/test/Microsoft.AspNetCore.Http.Tests/HttpContextAccessorTests.cs @@ -44,7 +44,6 @@ public async Task HttpContextAccessor_GettingHttpContextReturnsNullHttpContextIf var accessor = new HttpContextAccessor(); var context = new DefaultHttpContext(); - context.TraceIdentifier = "1"; accessor.HttpContext = context; var checkAsyncFlowTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -76,7 +75,6 @@ public async Task HttpContextAccessor_GettingHttpContextReturnsNullHttpContextIf // Null out the accessor accessor.HttpContext = null; - context.TraceIdentifier = null; waitForNullTcs.SetResult(null); @@ -86,12 +84,11 @@ public async Task HttpContextAccessor_GettingHttpContextReturnsNullHttpContextIf } [Fact] - public async Task HttpContextAccessor_GettingHttpContextReturnsNullHttpContextIfDifferentTraceIdentifier() + public async Task HttpContextAccessor_GettingHttpContextReturnsNullHttpContextIfChanged() { var accessor = new HttpContextAccessor(); var context = new DefaultHttpContext(); - context.TraceIdentifier = "1"; accessor.HttpContext = context; var checkAsyncFlowTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -121,12 +118,8 @@ public async Task HttpContextAccessor_GettingHttpContextReturnsNullHttpContextIf await checkAsyncFlowTcs.Task; - // Reset the trace identifier on the first request - context.TraceIdentifier = null; - // Set a new http context var context2 = new DefaultHttpContext(); - context2.TraceIdentifier = "2"; accessor.HttpContext = context2; waitForNullTcs.SetResult(null); @@ -142,7 +135,6 @@ public async Task HttpContextAccessor_GettingHttpContextDoesNotFlowIfAccessorSet var accessor = new HttpContextAccessor(); var context = new DefaultHttpContext(); - context.TraceIdentifier = "1"; accessor.HttpContext = context; var checkAsyncFlowTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -172,7 +164,6 @@ public async Task HttpContextAccessor_GettingHttpContextDoesNotFlowIfExecutionCo var accessor = new HttpContextAccessor(); var context = new DefaultHttpContext(); - context.TraceIdentifier = "1"; accessor.HttpContext = context; var checkAsyncFlowTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); diff --git a/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs b/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs index 80e42127..56b996f5 100644 --- a/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs +++ b/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs @@ -34,7 +34,6 @@ public void DisposeHttpContextSetsHttpContextAccessorToNull() // Act var context = contextFactory.Create(new FeatureCollection()); - var traceIdentifier = context.TraceIdentifier; // Assert Assert.Same(context, accessor.HttpContext); @@ -42,7 +41,6 @@ public void DisposeHttpContextSetsHttpContextAccessorToNull() contextFactory.Dispose(context); Assert.Null(accessor.HttpContext); - Assert.NotEqual(traceIdentifier, context.TraceIdentifier); } [Fact] From baf4b0e1e09a77aed046e862ebdf82f88bcd65e4 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 16 Nov 2018 06:14:45 +0000 Subject: [PATCH 3/4] add comment --- src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs index 08b4d6f9..e17d08a9 100644 --- a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs +++ b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs @@ -32,6 +32,7 @@ public HttpContext HttpContext } else if (holder != null) { + // Only want to set it to null if its already set; as setting to null is still a value and will move off default context _httpContextCurrent.Value = null; } } From a0d8966a32784b36aeb38f76b19427602e507462 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 16 Nov 2018 06:24:19 +0000 Subject: [PATCH 4/4] holder will get dumped anyway --- src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs index e17d08a9..28615102 100644 --- a/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs +++ b/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs @@ -30,11 +30,6 @@ public HttpContext HttpContext // so it can be cleared in all ExecutionContexts when its cleared. _httpContextCurrent.Value = new HttpContextHolder { Context = value }; } - else if (holder != null) - { - // Only want to set it to null if its already set; as setting to null is still a value and will move off default context - _httpContextCurrent.Value = null; - } } }