SetupSequence() is not thread-safe

[icalvo]

SetupSequence() is not thread-safe. If two threads call the mock, it can return the same value more than once. The following failing test demonstrate it:

using System.Collections.Concurrent;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;

namespace MoqTests
{
    [TestClass]
    public class SetupSequenceTests
    {
        private interface ITest
        {
            int Method();
        }

        [TestMethod]
        public async Task Test()
        {
            const int length = 20;

            var m = new Mock<ITest>();

            // Setups the mock to return a sequence of results.
            var seqSetup = m.SetupSequence(x => x.Method());
            for (int i = 1; i <= length; i++)
            {
                seqSetup = seqSetup
                    .Returns(i);
            }
            
            // Queue to store the invocations of the mock.
            var invocationsQueue = new ConcurrentQueue<int>();

            // We will invoke the mock from two different tasks.
            var t1 = Task.Run(() => RegisterInvocations(m, invocationsQueue));
            var t2 = Task.Run(() => RegisterInvocations(m, invocationsQueue));
            await Task.WhenAll(t1, t2);

            var orderedInvocations = invocationsQueue.OrderBy(x => x).ToArray();

            // The assertion prints the real invocations, you will see duplicates there!
            CollectionAssert.AreEqual(
                Enumerable.Range(1, length).ToArray(),
                orderedInvocations,
                string.Join(", ", orderedInvocations.Select(x => x.ToString())));
        }

        // Calls the mock until it returns default(int)
        private static void RegisterInvocations(IMock<ITest> m, ConcurrentQueue<int> invocationsQueue)
        {
            int result;
            do
            {
                result = m.Object.Method();
                if (result != default(int))
                {
                    invocationsQueue.Enqueue(result);
                }
            } while (result != default(int));
        }
    }
}

[stakx]

Hi @icalvo, and thanks for reporting this. Would you like to attempt a fix? If so, please feel free to send a PR (against the develop branch).

Otherwise, I'll take a look at this sometime during the next couple of days.

[stakx]

Note to self:

SetupSequence is currently built on top of several other parts of the public API (When, Callback, Throws and Returns). The thread-safety issue reported here exists because comparing the invocation count ("current step number") happens in When, but only gets increased later, in a Callback... that is, the increment doesn't happen atomically.

In order to make SetupSequence thread-safe, it seems likely that this approach must be given up. Instead, a new MethodCall subclass needs to be implemented that overrides Execute to do all of these things at once. Then there'll be a single site where we can ensure increments are atomic, etc.

[stakx]

@icalvo - A new implementation of SetupSequence is ready to be merged, but thread-safety comes at the cost of a functional, breaking change: SetupSequence will now override preexisting setups for identical invocations, where previously it didn't.

I'm actually fine with this, because it brings SetupSequence more in line with how the other Setup methods already work.

People might have relied on the previous (current) funky behaviour of Moq to first setup a default action or return value for an invocation, then partially override that default with a second sequence setup. Once the sequence is exhausted, invocations would default / fall back to the first setup. That will no longer work if the above PR gets merged.

What's probably missing with the new behaviour is methods on sequences such as .ReturnsByDefault and .ThrowsByDefault (or pick any other name) that define what the default response for an exhausted sequence should be. This can easily be added (now, or later).

[rusanov-vladimir]

While this issue isn't solved I've used approach Haacked blogged about.
This extension method seems to be working fine in parallel scenarios:

 public static class MoqExtensions
    {
        public static void ReturnsInOrder<T, TResult>(this ISetup<T, TResult> setup, params object[] results) where T : class
        {
            var queue = new ConcurrentQueue<object>(results);
            setup.Returns(() =>
            {
                object result;
                queue.TryDequeue(out result);
                if (result is Exception)
                {
                    throw result as Exception;
                }
                return (TResult)result;
            });
        }
    }

[stakx]

@icalvo, @rusanov-vladimir - I just published a pre-release version 4.8.0-rc1 on NuGet. It contains (among other things) a re-implementation of SetupSequence that should now be thread-safe.

[zxxc]

Hi, we are using 4.8.1 and issue it still persists
_client.SetupSequence(c => c.Get(It.IsAny(), It.IsAny()))
.ReturnsAsync(resp1)
.ThrowsAsync(exceptionForBrokenRequest);
Is this issue fixed for Throws and ThrowsAsync?

[stakx]

Can you provide a minimal code example that reproduces the issue you're referring to? Otherwise it's a little hard to tell what precisely we're talking about.

[zxxc]

Sorry it's my fault. I was using it wrong
I need for particular values throw exception
And for other return value
And i'm starting it in parallel

seems _client.SetupSequence() is not suitable for this
and i need to combine somehow two lines
_client.Setup(c=>c.Get(1)).Returns(id=> return dic[id]);

_client.Setup(c=>c.Get(2)).Throws();