remove subscriptions race

[TyOverby]

Fix Issue # .

• I ran Unit Tests locally.

For significant contributions please make sure you have completed the following items:

• Changes in public surface reviewed

• Design discussion issue #

• CHANGELOG.md updated with one line description of the fix, and a link to the original issue.

• The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
  If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)

• Please follow [these] (https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/develop/Readme.md) instructions to build and test locally.

[TyOverby]

Q: Should this Dispose call be here?

If we are the "owners" of value then I think it should be disposed, but is it possible that this value is shared?

[lmolkova]

no, we do not need to dispose the listener object itself. we dispose subscription (result of value.SubscribeWithAdapter)

[TyOverby]

I'll fix this, thanks for answering!

[lmolkova]

please create a new object to lock on

[TyOverby]

Sounds good

[lmolkova]

please make it volatile

[TyOverby]

Done.

[lmolkova]

it seems we don't need a lock around a loop in dispose as we are rejecting new subscriptions (in OnNext) based on the shuttingDown value.

so we can remove all locks around loops (in OnNext and here). Lock around shuttingDown assignment is needed only if it's not volatile.

Another reason to lock in OnNext is concurrent add and List is not concurrent.

So my proposal is:

• use a concurrent collection of subscriptions instead of List(bag or queue, it seems any would work)
• make shuttingDown volatile
• remove all locks

[TyOverby]

@lmolkova I've made the request changes, however, I still believe that a lock is necessary.

Imagine the scenario:

• <Thread 1> Disposal is called on the object
• <Thread 1> shuttingDown is set to true
• <Thread 2> OnNext is called on the object
• <Thread 2> shuttingDown is read as false (the above write happens concurrently with this read)
• <Thread 1> Subscriptions is looped over and its elements disposed
• <Thread 2> A new subscription is added to Subscriptions

With this model we mitigate the iterator invalidation exception (by making it use ConcurrentBag), but we run the risk of not disposing some elements in the bag (if those elements are added while Disposal is happening.

[pharring]

Suggestion:
Don't use a separate 'bool shuttingDown'.

Instead, remove readonly from subscriptions and, in Dispose(bool disposing) do this:

if (!disposing)
{
    return;
}

var subs = Interlocked.Exchange(ref this.subscriptions, null);
if (subs != null)
{
    foreach (var sub in subs) { sub.Dispose(); }
}

And, similarly, in OnNext:

var sub = Volatile.Read(ref this.subscirptions); // Could also do Interlocked.CompareExchange(ref this.subscriptions, null, null) to 'read' the current value
if (sub == null)
{
    // Shutting down
    return;
}

[cijothomas]

LG for me. It'd be great if you can include Paul's comment as well :)

[cijothomas]

@TyOverby Also - please update Changelog.md with a 1 liner about this (and link to the original GH issue you opened)
This will be part of 2.3.0-beta2

[TyOverby]

@cijothomas: done!