search

microsoft / ApplicationInsights-dotnet-server

Microsoft Application Insights for .NET Web Applications
https://azure.microsoft.com/services/application-insights/
133 stars 67 forks source link

Replace non-threadsafe HashSet with ConcurrentDictionary in RequestTrackingTelemetryModule.IsHandlerToFilter #1211

Closed Mikhail-msft closed 5 years ago

Mikhail-msft commented 5 years ago

Fix Issue # . Fixes IndexOutOfRangeException: WAC FrontEnd unhandled exception [0] [ExceptionType:System.IndexOutOfRangeException] System.IndexOutOfRangeException: Index was outside the bounds of the array. at System.Collections.Generic.HashSet`1.AddIfNotPresent(T value) at Microsoft.ApplicationInsights.Web.RequestTrackingTelemetryModule.IsHandlerToFilter(IHttpHandler handler) at Microsoft.ApplicationInsights.Web.RequestTrackingTelemetryModule.NeedProcessRequest(HttpContext httpContext) ...

TimothyMothra commented 5 years ago

In general, this looks good to me. Is HashSet thread safe? I think that was the main reason for using ConcurrentDictionary here.

cijothomas commented 5 years ago

In general, this looks good to me. Is HashSet thread safe? I think that was the main reason for using ConcurrentDictionary here.

https://stackoverflow.com/a/18923091/2624380

TimothyMothra commented 5 years ago

In general, this looks good to me. Is HashSet thread safe? I think that was the main reason for using ConcurrentDictionary here.

https://stackoverflow.com/a/18923091/2624380

lol. I was just reading the same article! :) I haven't seen the exception that @Mikhail-Pranovich referenced, so it would be good to hear what problem he's trying to solve.

Mikhail-msft commented 5 years ago

And modify changelog.md to add one line about this bug fix.

@cijothomas, Do we need to modify it if the fix is within same version? (i.e. this code was not shipped before)

TimothyMothra commented 5 years ago

oooohhhhhhhhhhh. i read this backwards.. it's replacing the HashSet with ConcurrentDictionary. I agree with this change

jirigregor commented 5 years ago

Is there any chance to release this fix in some quicker way then usual several months cycle? I consider it as a critial bug causing all of our application crashing randomly and there is no other way how to fix it than restarting entire application (in IIS). So currently we had to turn off ApplicationInsights completely.

TimothyMothra commented 5 years ago

Hi @jirigregor This fix is available now in our 2.11-beta1 release.

Regarding our release cadence, we currently release something monthly. Currently, our requirements are that we only ship a patch release if we introduced a regression. NEW bug fixes are picked up in the next version's release cycle. But I've seen a handful of requests asking for quicker bug fixes. I'll bring this up in our standup today.