Call assertions sometimes fail for concurrent calls

[ToniRiegler]

There seems to be a bug using MustHaveHappened in combination with Tasks.

Please see my gist:
https://gist.github.com/ToniRiegler/4812345401f582755e876349978151e2

It doesn't happen all of the time, but you can reproduce it easily using NCrunch with Churn selected test until it fails activated.

The Bug is reproducible with FakeItEasy 4.7.1 and 4.9.0 but not with FakeItEasy 2.3.1.

[thomaslevesque]

Hi @ToniRiegler,

Thanks for reporting, we'll look into it. Which framework are you targeting? .NET Framework, .NET Core ? Which version?

[ToniRiegler]

.NET Framework 4.7

[thomaslevesque]

I can repro after a few iterations (usually less than 50). Looks like a race condition where we record calls. But I'm not sure how it can happen... We're using a ConcurrentQueue for this, which should be perfectly thread-safe... Still looking.

[thomaslevesque]

OK, I found the root cause of the problem. The condition in the Count here:

FakeItEasy/src/FakeItEasy/Core/FakeAsserter.cs

Line 34 in 7a5fd74

var matchedCallCount = this.calls.Count(c => IsBeforeAssertionStart(c) && callPredicate(c));

excludes the first call, because it has a higher sequence number than the second. This happens because of a race condition in FakeManager.RecordCall:

FakeItEasy/src/FakeItEasy/Core/FakeManager.cs

Lines 179 to 183 in 7a5fd74

	internal void RecordCall(ICompletedFakeObjectCall call)
	{
	SequenceNumberManager.RecordSequenceNumber(call);
	this.recordedCalls.Enqueue(call);
	}

The race condition is something like this:

1. [Thread1] assign sequence number for call 1
2. [Thread2] assign sequence number for call 2
3. [Thread2] add call 2 to recorded calls
4. [Thread1] add call 1 to recorded calls

Possible fixes:

1. Put a lock around the two statements in FakeManager.RecordCalls. I tested it, it works, but it's not ideal as it might impact performance.
2. In FakeAsserter.AssertWasCalled, take the max sequence number instead of the last. Also tested, also works. Performance impact is probably smaller, unless there are many calls.
3. Keep track of the highest recorded sequence number for that fake. Almost no performance impact, but I'm not sure we can do it in a thread-safe way without using a lock.

[blairconrad]

This change has been released as part of FakeItEasy 4.9.1.

Thanks, @ToniRiegler! The reproduction instructions were excellent. Look for your name in the release notes. 🏆

[ToniRiegler]

I'm glad you found it.
Thanks for new version and mention in the release notes.

[blairconrad]

Our pleasure (easy for me to say; @thomaslevesque did the hard work). Happy FakeItEasying.