Middleware throws Argument Exception if cached headers conflict with pre-existing response headers
BrianVallelunga opened this issue · 7 comments
I just encountered an error while using ResponseCache in conjunction with the Stackify APM extension on Azure. It seems something is trying to insert a duplicate X-StackifyID header. I'm not sure if it is the extension that's misbehaving, or ResponseCache, or just a combination that hadn't been thought of.
My use of the ResponseCache is pretty trivial: [ResponseCache(Duration = 30)]
The error does not occur every time, but it's pretty easy to reproduce.
exception": "System.ArgumentException: An item with the same key has already been added. Key: X-StackifyID\r\n at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(Object key)\r\n at System.Collections.Generic.Dictionary2.Insert(TKey key, TValue value, Boolean add)\r\n at System.Collections.Generic.Dictionary2.Add(TKey key, TValue value)\r\n at Microsoft.AspNetCore.Server.Kestrel.Internal.Http.FrameResponseHeaders.AddValueFast(String key, StringValues value)\r\n at Microsoft.AspNetCore.Server.Kestrel.Internal.Http.FrameHeaders.System.Collections.Generic.IDictionary<System.String,Microsoft.Extensions.Primitives.StringValues>.Add(String key, StringValues value)\r\n at Microsoft.AspNetCore.Server.Kestrel.Internal.Http.FrameHeaders.System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<System.String,Microsoft.Extensions.Primitives.StringValues>>.Add(KeyValuePair2 item)\r\n at Microsoft.AspNetCore.ResponseCaching.ResponseCachingMiddleware.d__10.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n at System.Runtime.CompilerServices.TaskAwaiter1.GetResult()\r\n at Microsoft.AspNetCore.ResponseCaching.ResponseCachingMiddleware.d__11.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---
I ran into the same issue as well. I had some discussion with the Stackify guys via their support channels as I wasn't sure if the issue was in their code or in this middleware.
I'm also not sure if it is the site extension or their logging extension or their RequestTracerMiddleware. I removed the extension, but still see the issue.
I can confirm the problem is related to adding app.UseMiddleware<StackifyMiddleware.RequestTracerMiddleware>(); to my asp.net core 1.1 app.
There are a few issues here it seems.
First, response caching middleware assumes no response headers are set when it tries to serve a cached response. It seems like this is not the case here. What seems to be happening is that a preceding middleware is setting the X-StackifyID header on the response. Subsequently, the response caching middleware is causing an exception when it tries to add the same header. This is a bug that should be fixed in the response caching middleware since we should not be causing exceptions.
We are addressing this issue by overwriting pre-existing response headers when serving from cache. Note that the ordering of the middleware operations is wrong in this case. Previous middlewares in the pipeline should wait for until response caching has updated the response headers before making their own modifications, not before.
That said, this also brings into attention a few other issues:
What should be happening here is that the response is cacheable but the X-StackifyID header should probably be stripped from the response. This functionality is defined in the spec using no-cache fields RFC 7234. The issue for adding this feature is #40.
However, since there may be additional modifications that the users want to make on storing a response and serving a cached response. We will investigate what kinds of extensibility we can add to achieve this in #52.
@JunTaoLuo could you file two more bugs (for 1.0.1 and 1.1.1) for patch candidates?
@Eilon this is a low-risk change and since we already have patches approved for ResponseCaching, I think we should consider this one too.
Thanks for the work. It would be great to get this in 1.0 and 1.1. It seems the fix was trivial.
@BrianVallelunga we're going to try to get it into those patch releases if we can.