Skip to content
This repository was archived by the owner on Jul 5, 2020. It is now read-only.

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

Merged
Mikhail-msft merged 2 commits intodevelopfrom
mikp/ConcurrentHashset
Jun 4, 2019
Merged

Replace non-threadsafe HashSet with ConcurrentDictionary in RequestTrackingTelemetryModule.IsHandlerToFilter#1211
Mikhail-msft merged 2 commits intodevelopfrom
mikp/ConcurrentHashset

Conversation

@Mikhail-msft
Copy link
Copy Markdown
Contributor

@Mikhail-msft Mikhail-msft commented May 30, 2019

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)
...

  • I ran Unit Tests locally.

Copy link
Copy Markdown
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a descriptive title, and explain how this change is fixing the issue.
And modify changelog.md to add one line about this bug fix.

@TimothyMothra
Copy link
Copy Markdown

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

@cijothomas
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown

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 Mikhail-msft changed the title merge Replace non-threadsafe HashSet with ConcurrentDictionary in RequestTrackingTelemetryModule.IsHandlerToFilter May 30, 2019
@Mikhail-msft
Copy link
Copy Markdown
Contributor Author

Mikhail-msft commented May 30, 2019

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
Copy link
Copy Markdown

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

* readonly
* changelog.md update
@Mikhail-msft Mikhail-msft merged commit 1296a66 into develop Jun 4, 2019
@Mikhail-msft Mikhail-msft deleted the mikp/ConcurrentHashset branch June 4, 2019 17:24
@jirigregor
Copy link
Copy Markdown

jirigregor commented Aug 22, 2019

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
Copy link
Copy Markdown

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants