Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix new race condition #144

Merged
merged 2 commits into from
Jan 28, 2020

Conversation

marcoregueira
Copy link
Contributor

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

stimms commented Dec 19, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

marcoregueira commented Dec 20, 2018

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
Copy link
Contributor Author

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.

@dpaquette dpaquette merged commit 8ed4f44 into MisterJames:master Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants