MisterJames / GenFu

GenFu is a library you can use to generate realistic test data. It is composed of several property fillers that can populate commonly named properties through reflection using an internal database of values or randomly created data. You can override any of the fillers, give GenFu hints on how to fill them.
Other
831 stars 100 forks source link

Fix new race condition #144

Closed marcoregueira closed 4 years ago

marcoregueira commented 5 years ago

This code fixes a new race condition in Filler manager that appeared since #133 was created, regarding generic fillers.

Note that parallel tests and all the other tests should not be executed together. Parallel tests reset registrations interfering other tests.

stimms commented 5 years ago

Not being able to run the entire test suite at the same time is problematic. It is, I suppose, a symptom of the larger problem that is global registrations. Even running registrations_can_be_reset alone on my machine causes an exception.

[12/19/2018 11:58:04 AM Error] [xUnit.net 00:00:02.9397080]     GenFu.Tests.When_running_in_parallel.registrations_can_be_reset_separately [FAIL]
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9422081]       System.AggregateException : One or more errors occurred. (The given key was not present in the dictionary.)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9423414]       ---- System.Collections.Generic.KeyNotFoundException : The given key was not present in the dictionary.
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9435417]       Stack Trace:
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9453763]            at System.Threading.Tasks.TaskReplicator.Run[TState](ReplicatableUserAction`1 action, ParallelOptions options, Boolean stopOnFirstFailure)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9455888]            at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9456943]         --- End of stack trace from previous location where exception was thrown ---
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9458837]            at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9459700]            at System.Threading.Tasks.Parallel.ThrowSingleCancellationExceptionOrOtherException(ICollection exceptions, CancellationToken cancelToken, Exception otherException)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9460795]            at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9461656]            at System.Threading.Tasks.Parallel.For(Int32 fromInclusive, Int32 toExclusive, Action`1 body)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9463047]         C:\code\Play\GenFu\tests\GenFu.Tests\When_running_in_parallel.cs(31,0): at GenFu.Tests.When_running_in_parallel.registrations_can_be_reset_separately()
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9470853]         ----- Inner Stack Trace -----
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9472982]            at System.ThrowHelper.ThrowKeyNotFoundException()
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9474168]            at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9476269]         C:\code\Play\GenFu\src\GenFu\FillerManager.cs(331,0): at GenFu.FillerManager.GetGenericFiller[Input,Result]()
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9478798]         C:\code\Play\GenFu\src\GenFu\GenericFillerDefaults.cs(17,0): at GenFu.GenericFillerDefaults.SetMinInt(Int32 min)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9479781]         C:\code\Play\GenFu\src\GenFu\GenFuFluent.cs(62,0): at GenFu.GenFu.Reset()
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9481050]         C:\code\Play\GenFu\tests\GenFu.Tests\When_running_in_parallel.cs(36,0): at GenFu.Tests.When_running_in_parallel.<>c.<registrations_can_be_reset_separately>b__2_0(Int32 i)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9482280]            at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`1.<ForWorker>b__1(RangeWorker& currentWorker, Int32 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9483412]         --- End of stack trace from previous location where exception was thrown ---
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9484415]            at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9488569]            at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`1.<ForWorker>b__1(RangeWorker& currentWorker, Int32 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9490296]            at System.Threading.Tasks.TaskReplicator.Replica`1.ExecuteAction(Boolean& yieldedBeforeCompletion)
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9492036]            at System.Threading.Tasks.TaskReplicator.Replica.Execute()
[12/19/2018 11:58:04 AM Informational] [xUnit.net 00:00:02.9684566]   Finished:    GenFu.Tests

So I'm thinking about a mechanism where we take a copy of the configuration in each thread and only update that thread's version on a reset. Make configuration ThreadStatic then clone it when a thread without its own configuration starts up. On reset we increment a version counter and when Reset is called in each thread we could reclone. Thoughts?

marcoregueira commented 5 years ago

As I said in my previous comment:

Note that parallel tests and all the other tests should not be executed together. Parallel tests reset registrations interfering other tests.

This is solved in the new PR allowing genfu instances.

It is likely that threadstatic configurations would not work properly with 'async' operations anyway.

marcoregueira commented 5 years ago

I can see from your log you're still testing PR #133. You should use PR #144, that updates support for the new code added since the original PR was created.

(I can see that because the sentence return (Result)_genericPropertyFillersByPropertyType[type] was in line 331, but it should be 334 now, if you check the stack trace.)

I have not been able to replicate any error running registrations_can_be_reset. Not with vs nor rider (they seem to have a different behaviour when running tests in parallel). I can confirm the Parallel.For is running at least 10 different threads for that concrete test without interference.

marcoregueira commented 5 years ago

Aside of that, using Reset on the global registration list should interfere other threads, I don't see any wrong in that and fighting against that would be a waste of resources. Operations should be thread safe, meaning that a Reset should not fail in threaded scenarios. Obviously, what you will find in the list if you do that kind of things is non deterministic: you can find your data or some other thread's data.

For that reason I think it is much better to use instances, instead a global instance (or static class). That's the reason for PR #146, that allows both use cases.