Crash due to setting TCS result twice

[kentcb]

Having upgraded an older project with lots of tests to the latest device runner, I'm getting an exception on this line of DeviceRunner.cs. The debugging experience in VS+Xamarin is still terrible, but I managed to get this poorly-formatted stack trace:

"  at System.Threading.Tasks.TaskCompletionSource`1[System.Object].SetResult (System.Object result) [0x0000c] in /Users/builder/data/lanes/2320/1f068b49/source/maccore/_build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/src/mono/external/referencesource/mscorlib/system/threading/Tasks/TaskCompletionSource.cs:322 \n  at Xunit.Runners.DeviceRunner+<>c__DisplayClass17_0.<RunTests>b__0 () [0x000f9] in C:\\BuildAgent\\work\\a3e04f08310ca743\\src\\xunit.runner.viewmodels\\DeviceRunner.cs:254 \n  at Xunit.Runners.ThreadPoolHelper+<>c__DisplayClass0_0.<RunAsync>b__0 (System.Object _) [0x00000] in C:\\BuildAgent\\work\\a3e04f08310ca743\\src\\xunit.runner.ios-unified\\ThreadPoolHelper.cs:14 "

I tried at least narrowing down the test that was causing the problem by writing a BeforeAfterTestAttribute that spat out the test name before and after execution. However, this then causes all tests to pass! So it must be some kind of race condition.

Before I go digging too deep, any ideas what might cause this?

[clairernovotny]

That's really strange...I'm not sure where the bug is here. Looking at that method, it appears that a new TaskCompletionSource is created exactly once and then the handler method is executed on the threadpool once. The SetResult method is called once in the finally block.

It looks like we need to know what their version of TaskCompletionSource is doing on line 322.

[clairernovotny]

It's probably this line: https://github.com/mono/referencesource/blob/mono/mscorlib/system/threading/Tasks/TaskCompletionSource.cs#L322

[clairernovotny]

One theory: it's hitting SetException is called prior to SetResult and is thus not allowed.... do you see any first chance exceptions logged that might indicate this?

[bradwilson]

Yep, that line should be TrySetResult instead of SetResult, because the latter conflicts with the SetException call in the catch block.

[kentcb]

I've got break on all exceptions turned on, and this exception is the first one to show up :/

Thanks for making the fix @onovotny . Any idea when you'll be releasing to NuGet?

[kentcb]

OK, I found the underlying problem. I had a theory with many test cases, but one of them was repeated. For example:

[Theory]
[InlineData("a")]
[InlineData("b")]
[InlineData("c")]
[InlineData("d")]
[InlineData("a")]  // oops
[InlineData("e")]
public void theory(string input)
{
}

This was triggering an exception which VS+Xamarin was refusing to show me yesterday:

        $exception.StackTrace   "  at System.ThrowHelper.ThrowArgumentException (ExceptionResource resource) [0x00000] in /Users/builder/data/lanes/2320/1f068b49/source/maccore/_build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/src/mono/external/referencesource/mscorlib/system/throwhelper.cs:74 \n  at System.Collections.Generic.Dictionary`2[System.String,Xunit.Runners.TestCaseViewModel].Insert (System.String key, Xunit.Runners.TestCaseViewModel value, Boolean add) [0x0008e] in /Users/builder/data/lanes/2320/1f068b49/source/maccore/_build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/src/mono/external/referencesource/mscorlib/system/collections/generic/dictionary.cs:329 \n  at System.Collections.Generic.Dictionary`2[System.String,Xunit.Runners.TestCaseViewModel].Add (System.String key, Xunit.Runners.TestCaseViewModel value) [0x00000] in /Users/builder/data/lanes/2320/1f068b49/source/maccore/_build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/src/mono/external/referencesource/mscorlib/system/collections/generic/dictionary.cs:185 \n  at System.Linq.Enumerable.ToDictionary[<>f__AnonymousType0`2,String,TestCaseViewModel] (IEnumerable`1 source, System.Func`2 keySelector, System.Func`2 elementSelector, IEqualityComparer`1 comparer) [0x0002f] in /Users/builder/data/lanes/2320/1f068b49/source/maccore/_build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/src/mono/mcs/class/System.Core/System.Linq/Enumerable.cs:2924 \n  at System.Linq.Enumerable.ToDictionary[<>f__AnonymousType0`2,String,TestCaseViewModel] (IEnumerable`1 source, System.Func`2 keySelector, System.Func`2 elementSelector) [0x00000] in /Users/builder/data/lanes/2320/1f068b49/source/maccore/_build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/src/mono/mcs/class/System.Core/System.Linq/Enumerable.cs:2911 \n  at Xunit.Runners.DeviceRunner.RunTestsInAssembly (System.Collections.Generic.List`1 toDispose, Xunit.Runners.AssemblyRunInfo runInfo) [0x00043] in C:\\BuildAgent\\work\\a3e04f08310ca743\\src\\xunit.runner.viewmodels\\DeviceRunner.cs:276 \n  at Xunit.Runners.DeviceRunner+<>c__DisplayClass17_1.<RunTests>b__4 (Xunit.Runners.AssemblyRunInfo runInfo) [0x00000] in C:\\BuildAgent\\work\\a3e04f08310ca743\\src\\xunit.runner.viewmodels\\DeviceRunner.cs:243 \n  at Xunit.Runners.Extensions.ForEach[AssemblyRunInfo] (IEnumerable`1 This, System.Action`1 action) [0x00010] in C:\\BuildAgent\\work\\a3e04f08310ca743\\src\\xunit.runner.viewmodels\\Utilities\\Extensions.cs:14 \n  at Xunit.Runners.DeviceRunner+<>c__DisplayClass17_0.<RunTests>b__0 () [0x0009e] in C:\\BuildAgent\\work\\a3e04f08310ca743\\src\\xunit.runner.viewmodels\\DeviceRunner.cs:242 "    string

This was in turn triggering the SetException call and then subsequently the SetResult call, which triggered the original exception I mentioned.

Since I've had these tests working successfully in previous xUnit + devices incarnations, it is presumably a change of behavior. Is that right, @bradwilson ?

[clairernovotny]

I just published 1.2.2 with this fix. It was supposed to be 1.2.1 but I mistyped the push from MyGet :(

[kentcb]

@onovotny thanks!