Mutagen-Modding / Mutagen

A .NET library for analyzing, creating, and manipulating Bethesda mods
GNU General Public License v3.0
115 stars 32 forks source link

Something not entirely threadsafe around LoquiRegistration. #180

Open mrudat opened 3 years ago

mrudat commented 3 years ago

I suspect that there's some sort of race around LoquiRegistration; this only happens when unit tests in my branch of Synthesis are run in parallel, putting more overall load on the system. It does not trigger on the very similar test which applies AwesomeNPC and AnotherNPC in the other order, nor does it trigger when the test is run in serial, or under the debugger.

Test Source

Test Name:  Synthesis.Bethesda.UnitTests.PersistentState_Tests.FreshStart_AnotherNPCAndAwesomeNPC
Test FullName:  Synthesis.Bethesda.UnitTests.Synthesis.Bethesda.UnitTests.PersistentState_Tests.Synthesis.Bethesda.UnitTests.PersistentState_Tests.FreshStart_AnotherNPCAndAwesomeNPC
Test Source:    C:\Users\...\Documents\GitHub\Mutagen-Modding\Synthesis\Synthesis.Bethesda.UnitTests\PersistentState_Tests.cs : line 167
Test Outcome:   Failed
Test Duration:  0:00:00

Test Name:  Synthesis.Bethesda.UnitTests.PersistentState_Tests.FreshStart_AnotherNPCAndAwesomeNPC
Test Outcome:   Failed
Result StackTrace:  
at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at Loqui.LoquiRegistration.Register(ILoquiRegistration reg)
   at Loqui.ProtocolDefinition_Skyrim.Register()
   at Loqui.ProtocolDefinition_Skyrim.Loqui.IProtocolRegistration.Register()
   at Loqui.LoquiRegistration.Register(IProtocolRegistration[] registrations)
   at Loqui.Initialization.SpinUp(IProtocolRegistration[] registrations)
   at Mutagen.Bethesda.Skyrim.WarmupSkyrim.Init()
   at Mutagen.Bethesda.WarmupAll.Init()
   at Mutagen.Bethesda.Synthesis.SynthesisPipeline.Run(RunSynthesisMutagenPatcher args, ModKey exportKey, RunPreferences preferences) in C:\Users\...\Documents\GitHub\Mutagen-Modding\Synthesis\Mutagen.Bethesda.Synthesis\SynthesisPipeline.cs:line 359
   at Synthesis.Bethesda.UnitTests.PersistentState_Tests.FreshStart_AnotherNPCAndAwesomeNPC() in C:\Users\...\Documents\GitHub\Mutagen-Modding\Synthesis\Synthesis.Bethesda.UnitTests\PersistentState_Tests.cs:line 174
--- End of stack trace from previous location ---
Result Message: System.ArgumentException : An item with the same key has already been added. Key: Mutagen.Bethesda.Skyrim.ISkyrimModGetter
Noggog commented 3 years ago

I think this is just a different manifestation of an issue that's been around for a while: xUnit doesn't isolate static variables well enough between tests.

I'll typically see it where it'll be like:

public readonly static SomeClass Instance = new SomeClass();

And then get an exception -> "Instance was null". How can it possibly be null? lol

Feels like xUnit is trying to compartmentalize and clean up random things for tests, and just doesn't do the static instance stuff properly.. and/or we have our tests misconfigured.

Most of the time the code looks just fine, and the tests just fail due to static variables being toyed with under the hood by the test env. Not sure what to do about that

Since it only bites fairly rarely (for me), it's been low priority to solve. If it happens when im pushing a version, I just rerun the job and it's fine.