Change node filters to use a concurrent list instead of a dictionary

[mikedennis]

This is a prerequisite for the work I'm doing in #692

ConcurrentDictionary does not maintain insertion order so it's usage in NodeFiltersCollection does not allow for ordering of Filters.

ThreadSafeList uses a list instead of ConcurrentDictionary and the locking of the collection is done manually. Currently I've only replaced the usage in NodeFiltersCollection but I have run all the tests using ThreadSafeList isntead of ThreadSafeCollection in all occurrences and all the same tests pass. So we may be able to replace other instances as well.

ThreadSafeList will allow insertion of duplicates whereas ThreadSafeCollection will not so that is a key difference to be aware of beyond the ordering difference.

[NicolasDorier]

to be thread safe you need to lock Behavior, make a copy of the list, and returning the enumerator of this copy.

[NicolasDorier]

But then, it is a performance issue because we are duplicating a list everytimes a message come. :(

[mikedennis]

I think we were also doing that before (this is from ThreadSafeCollection):

		public IEnumerator<T> GetEnumerator()
		{
			return _Behaviors.Select(k => k.Key).GetEnumerator();
		}

[mikedennis]

Since we are doing all this copying anyways... wondering whether we might be able to use some of the new Immutable collections like ImmutableList, been dying to find a reason to try them out 😄

[NicolasDorier]

_Behaviors.Select(k => k.Key).GetEnumerator() is not a copy.

[NicolasDorier]

An idea, is that you make an array that is a copy of _Behaviors. Everytimes you modify _Behaviors, you set the array to null, else you use it in GetEnumerator.

[mikedennis]

Thanks, yes that should work. Also researching this a bit it seems that some people have implemented functionality like this using a ConcurrentDictionary with an integer (item index) as the key which may be a reasonable approach as well.

[mikedennis]

Updated to reduce list copying for enumerator as suggested. Reran the tests and they still pass.

[NicolasDorier]

Please capture enumeratorList in a lock variable so we are sure GetEnumerator never returns null.

[NicolasDorier]

IEnumerator<T> enumerator = _EnumeratorList?.GetEnumerator();
if (enumerator == null)
{
   lock (_lock)
   {
       var behaviorsList = _Behaviors.ToList();
       _EnumeratorList = behaviorsList;
       enumerator =  behaviorsList.GetEnumerator();
   }
}
return enumerator;

So you lock only if really needed. And it is provable that it is impossible to get a null enumerator out of this function.

[NicolasDorier]

I am tempted to revert this. The build is very unstable since I merged this. CanMaintainChainWithSlimChainBehavior is failing here and there, I have no idea why.

[NicolasDorier]

Tried to refactor a bit, no luck.

[mikedennis]

Sure no worries - go ahead and revert and I'll do some more testing with it with that test.

[NicolasDorier]

I change back to ThreadSafeCollection for now.

[NicolasDorier]

I don't understand the issue. Tried replicate on my machine but that did not worked.

[zeptin]

@mikedennis is SynchronizedCollection<T> unsuitable for what you were trying to do here?

[mikedennis]

@zeptin I believe that's only .NET framework. Found the source code though, looks like it would be easy to port https://github.com/microsoft/referencesource/blob/3b1eaf5203992df69de44c783a3eda37d3d4cd10/System.ServiceModel/System/ServiceModel/SynchronizedCollection.cs

[zeptin]

@NicolasDorier I am unable to get that test to fail locally while using the ThreadSafeList. Can we give that modification another try?

[NicolasDorier]

yes, make a pr