Add concurrent access detection tests to Dictionary<TKey, TValue> by MarcoRossignoli · Pull Request #30515 · dotnet/corefx

MarcoRossignoli · 2018-06-19T16:11:38Z

contributes to https://github.com/dotnet/corefx/issues/30023

/cc @danmosemsft

MarcoRossignoli · 2018-06-19T16:13:34Z

+                 Assert.False(dic.GetType().GetGenericArguments()[0].IsValueType);
+                 Assert.Equal("ThrowInvalidOperationException_ConcurrentOperationsNotSupported", Assert.Throws<InvalidOperationException>(() => dic.Add(new DummyRefType() { Value = 1 }, new DummyRefType() { Value = 1 })).TargetSite.Name);
+                 Assert.Equal("ThrowInvalidOperationException_ConcurrentOperationsNotSupported", Assert.Throws<InvalidOperationException>(() => dic[new DummyRefType() { Value = 1 }]).TargetSite.Name);
+                 //Remove is not resilient yet


Remove are not yet resilient to infinite loops...we can merge this(if are good) and after enable tests or wait for coreclr perft tests/merge.

danmoseley · 2018-06-19T16:51:10Z

+                dic.Add(1, 1);
+
+                //break internal state
+                var entriesType = dic.GetType().GetField("_entries", BindingFlags.NonPublic | BindingFlags.Instance);


Could you please extract common code (eg the reflection code)

danmoseley · 2018-06-19T16:52:39Z

+        }
+    }
+
+    public class DummyRefType


Why do you need special types and comparers, it seems strings and ints should suffice?

because string is optimized...empty constructor use internal comparer https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs#L79

danmoseley · 2018-06-19T16:54:12Z

+            customThread.Start();
+
+            //Wait max 5 seconds, could loop forever
+            Assert.True(customThread.Join(TimeSpan.FromSeconds(5)));


I would make these timeouts large (eg 30 or 60 sec). Because if the machine is overloaded, they may fail in the passing case - which is bad. I have seen test machines occasionally be very slow. It is OK if they are large, because unless we regress, they will never hit the timeout. All we care about is the entire test run isn't brought down in that case. Each test library gets 20 minutes total.

danmoseley · 2018-06-19T16:56:03Z

+        [Fact]
+        public static void Add_DictionaryConcurrentAccessDetection_NullComparer_ValueTypeKey()
+        {
+            Thread customThread = new Thread(() =>


I'm probably spacing here -- but where is the concurrent access? There is only one thread accessing it?

i'm simulating "concurrent access" breaking internal structure as if concurrent access happened. How i can be "sure" that threads will break internals?Depends on some random factors...i cannot guarantee break in n seconds(at least in my career trying to reproduce concurrent issue is not so easy...samples work well on my machine and doesn't work on other, PerformanceCounter for instance...the behaviour are different on different machine with different number of processor, different parameters to fail test), i use thread only because if for some reason in future access detection will be removed threads doesn't loop forever blocking CI. I hope I've been clear.

Doh - of course - that was the plan and it's what you did. BTW, I wonder if you could await a Task instead of using a Thread - @ViktorHofer the Task/Thread is only to add a timeout - if we wait on a Task perhaps it won't be marshaled to another thread, so there effectively won't be a timeout?

Task perhaps it won't be marshaled to another thread, so there effectively won't be a timeout?

~~I could do that with Task.Run+Task.Delay+Task.WaitAll maybe...to simulate timeout...but maybe code will be less clear.~~

You could use a Task with the LongRunning option which forces the TaskScheduler to spin up a new thread.

yes i forget Wait(TimeSpan) overload.

@ViktorHofer @danmosemsft another idea could be use Thread with LowPriority https://source.dot.net/#System.Private.CoreLib/shared/System/Threading/ThreadPriority.cs,e968ef87c207562d, if test fail loop will be less heavy...long running task is a normal Thread https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Tasks/ThreadPoolTaskScheduler.cs#L47

please prefer using Task over Thread if possible.

ok i'll do!

At that point, would @MarcoRossignoli await the task, and make the test async? I suppose that doesn't give us much - the tests could overlap, but they should be very quick anyway.

ViktorHofer · 2018-06-19T18:18:16Z

+        public static void Add_DictionaryConcurrentAccessDetection_Comparer_ValueTypeKey()
+        {
+            Thread customThread = new Thread(() =>
+             {


nit: indentation

MarcoRossignoli · 2018-06-19T18:24:36Z

+            customThread.Start();
+
+            //Wait max 5 seconds, could loop forever
+            Assert.True(customThread.Join(TimeSpan.FromSeconds(5)));            


@danmosemsft In case of timeout and assertion fail thread will run "forever" however...but as background thread at the end of test will be killed. I avoided to call Kill/Abort by myself because there are no guarantee at least on win. We cannot pass some CancellationToken to internals.

A timeout causing an assertion failure sounds good to me.

MarcoRossignoli · 2018-06-20T11:37:46Z

@danmosemsft @ViktorHofer i've simplified test and added Task instead of Thread.

ViktorHofer · 2018-06-20T12:25:46Z

    </Compile>
    <!-- Generic tests -->
    <Compile Include="Generic\Dictionary\Dictionary.Generic.Tests.netcoreapp.cs" Condition="'$(TargetGroup)' == 'netcoreapp'" />
+    <Compile Include="Generic\Dictionary\Dictionary.Generic.Tests.ConcurrentAccessDetection.netcoreapp.cs" Condition="'$(TargetGroup)' == 'netcoreapp'" />


It would be great if you could put all netcoreapp condition compile directives into their own ItemGroup.
...

ViktorHofer · 2018-06-20T12:27:21Z

+        [Theory]
+        [InlineData(null)]
+        [InlineData(typeof(CustomEqualityComparerInt32ValueType))]
+        async public static Task Add_DictionaryConcurrentAccessDetection_ValueTypeKey(Type comparerType)


public async Task Add_DictionaryConcurrentAccessDetection_ValueTypeKey. Async should be after the access modifier and there's no need for the method to be static.

ok i used static because i'm working also on Span tests refactor...and in that project are all static..is there a guideline?

https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/ReadOnlySpan/SequenceEqual.char.cs

I and most of us prefer non-static test methods.

@karelz should we have a guideline for static vs non-static test methods?

I and most of us prefer non-static test methods.

Also me and my team.

I think Xunit runs the ctor for each test. So non static gives you a fresh instance. Otherwise I'm not sure we care, save consistency. @stephentoub?

Correct: https://xunit.github.io/docs/shared-context#constructor

So static class for tests are more performant.

ViktorHofer · 2018-06-20T12:28:49Z

+    }
+}
+
+public class DummyRefType


Why are those types not inside a namespace? Also please use the most restrictive access modifier possible, in you case either private or internal depending on where you place the types. This also applies to the custom equalitycomparers.

ViktorHofer · 2018-06-20T12:29:24Z

+
+public class DummyRefType
+{
+    public int Value { get; set; }


nit: separate methods and properties by a an empty line.

ViktorHofer · 2018-06-20T12:30:04Z

+        [Theory]
+        [InlineData(null)]
+        [InlineData(typeof(CustomEqualityComparerDummyRefType))]
+        async public static Task Add_DictionaryConcurrentAccessDetection_ReferenceTypeKey(Type comparerType)


ViktorHofer · 2018-06-20T12:31:36Z

+        {
+            IEqualityComparer<int> customComparer = null;
+
+            Dictionary<int, int> dic =


nit: we usually put the comparison into the same line and only the then and else clause into a new line.

ViktorHofer · 2018-06-20T12:33:18Z

+            Task task = Task.Factory.StartNew(() =>
+            {
+                //break internal state
+                var entriesType = dictionary.GetType().GetField("_entries", BindingFlags.NonPublic | BindingFlags.Instance);


nit: concrete type instead of var where it's not obvious on the right hand side of the declaration.

is this not obvious? 😄 you'are right, reflection is worste code for var!

danmoseley · 2018-06-20T14:12:51Z

+            {
+                //break internal state
+                FieldInfo entriesType = dictionary.GetType().GetField("_entries", BindingFlags.NonPublic | BindingFlags.Instance);
+                object entriesInstance = (Array)entriesType.GetValue(dictionary);


Nit, this can be declared Array

danmoseley · 2018-06-20T14:21:44Z

+        {
+            Task task = Task.Factory.StartNew(() =>
+            {
+                //break internal state


No need to fix unless you reset the PR again...we use single space after //
Also "break internal state" could be more explicit eg " set an empty entries array to simulate a dictionary corrupted by concurrent access".
Am I reading that right? I was assuming you would set an entries array with a loop, how does an empty one work?

Am I reading that right? I was assuming you would set an entries array with a loop, how does an empty one work?

is not empty

Array entryArray = (Array)Activator.CreateInstance(entriesInstance.GetType(), new object[] { ((IDictionary)dictionary).Count });

the idea is break alignement between states of _buckets and _entries "old" _buckets state avoid exit from loops and new _entries.next generate loop.

Ah, I see (now I'm not reading on the phone). The correct Bucket points to an Entry with .next of 0, so it loops.

danmoseley · 2018-06-20T14:22:48Z

+        }
+    }
+
+    class DummyRefType


Consider a comment explaining why you use custom types in your tests

Done...sorry for my very basic english...help me with mistakes please 😅

danmoseley

I tweaked the comment. Thanks @MarcoRossignoli

MarcoRossignoli · 2018-06-20T18:15:45Z

I tweaked the comment

Thank's and sorry for wasted time.

danmoseley · 2018-06-20T18:21:03Z

@MarcoRossignoli you didn't waste any of my time, you saved me time by doing this work.

danmoseley · 2018-06-20T18:32:44Z

blocked on dotnet/coreclr#18524

MarcoRossignoli · 2018-06-21T10:01:03Z

blocked on dotnet/coreclr#18524

I've enabled remove tests...i think is ready to go after green CI.

danmoseley · 2018-06-21T11:28:45Z

Thanks @MarcoRossignoli!

MarcoRossignoli · 2018-06-21T12:08:55Z

Thanks @MarcoRossignoli!

mmm...1.28 PM in Italy...deep night in Redmond...i'm suspecting you're a bot @danmosemsft-bot

…tnet/corefx#30515) * add concurrent access detection tests * add remove overloads * simplify tests * address PR feedback * remove var * nit extraline * move comparison into same line * cleanup unuseful instance * cleanup typo * improve readability * address PR feedback * Comment * Access modifier * enable removes tests Commit migrated from dotnet/corefx@24b9efc

MarcoRossignoli added 2 commits June 19, 2018 18:01

add concurrent access detection tests

828268f

add remove overloads

41cdfc8

MarcoRossignoli commented Jun 19, 2018

View reviewed changes

danmoseley reviewed Jun 19, 2018

View reviewed changes

ViktorHofer reviewed Jun 19, 2018

View reviewed changes

MarcoRossignoli commented Jun 19, 2018

View reviewed changes

simplify tests

15faf41

MarcoRossignoli changed the title ~~[WIP]Add concurrent access detection tests to Dictionary<TKey, TValue>~~ Add concurrent access detection tests to Dictionary<TKey, TValue> Jun 20, 2018

ViktorHofer requested a review from stephentoub June 20, 2018 12:23

ViktorHofer added the area-System.Collections label Jun 20, 2018

ViktorHofer assigned MarcoRossignoli Jun 20, 2018

ViktorHofer reviewed Jun 20, 2018

View reviewed changes

MarcoRossignoli added 7 commits June 20, 2018 14:51

address PR feedback

83b864a

remove var

f34e2c4

nit extraline

e2d1c52

move comparison into same line

176d9f6

cleanup unuseful instance

19540a1

cleanup typo

bf51b10

improve readability

1253a73

danmoseley reviewed Jun 20, 2018

View reviewed changes

MarcoRossignoli and others added 2 commits June 20, 2018 16:32

address PR feedback

4ce8758

Comment

e04d27a

danmoseley approved these changes Jun 20, 2018

View reviewed changes

Access modifier

902738a

enable removes tests

ec9d5df

danmoseley merged commit 24b9efc into dotnet:master Jun 21, 2018

MarcoRossignoli deleted the concurrentaccessdetectiontests branch June 21, 2018 12:09

karelz added this to the 3.0 milestone Jul 8, 2018

Conversation

MarcoRossignoli commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MarcoRossignoli Jun 19, 2018 •

edited

Loading

MarcoRossignoli Jun 19, 2018 •

edited

Loading

MarcoRossignoli Jun 19, 2018 •

edited

Loading

MarcoRossignoli Jun 19, 2018 •

edited

Loading

ViktorHofer Jun 20, 2018 •

edited

Loading

MarcoRossignoli Jun 20, 2018 •

edited

Loading