Autofixture: AutoConfiguredNSubstituteCustomization not working correctly in XUnit theories with parallel tasks

[MichaelLogutov]

We've been facting strange issues when running tests that invokes multiple tasks (to simulate multithreaded environment). I found that the issue is related to AutoConfiguredNSubstituteCustomization. Here is the test:

using System;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using NSubstitute;
using Ploeh.AutoFixture;
using Ploeh.AutoFixture.AutoNSubstitute;
using Xunit;

namespace Bug
{
    public interface IServiceA
    {
        IServiceB GetService();
    }

    public interface IServiceB
    {
    }


    public class Tests
    {
        [Theory]
        [InlineData(32), InlineData(64), InlineData(128), InlineData(256)]
        public Task UsingAutoConfiguredNSubstituteCustomization(int dop)
        {
            // arrange
            var fixture = new Fixture();
            fixture.Customize(new AutoConfiguredNSubstituteCustomization());

            var service = fixture.Freeze<IServiceA>();

            // act, assert
            return TestAsync(dop, service);
        }


        [Theory]
        [InlineData(32), InlineData(64), InlineData(128), InlineData(256)]
        public Task AutoNSubstituteCustomization(int dop)
        {
            // arrange
            var fixture = new Fixture();
            fixture.Customize(new AutoNSubstituteCustomization());

            var service = fixture.Freeze<IServiceA>();

            // act, assert
            return TestAsync(dop, service);
        }


        [Theory]
        [InlineData(32), InlineData(64), InlineData(128), InlineData(256)]
        public Task UsingNSubstitute(int dop)
        {
            // arrange
            var service = Substitute.For<IServiceA>(); // no error

            // act, assert
            return TestAsync(dop, service);
        }


        private static async Task TestAsync(int dop, IServiceA service)
        {
            // arrange
            var start = new SemaphoreSlim(0, dop);
            var cts = new CancellationTokenSource(TimeSpan.FromSeconds(1));

            var tasks = Enumerable
                .Range(0, dop)
                .Select(_ => Task.Run
                                 (async () =>
                                        {
                                            await start.WaitAsync(cts.Token).ConfigureAwait(false);
                                            service.GetService();
                                        },
                                  cts.Token));


            // act
            start.Release(dop);
            await Task.WhenAll(tasks).ConfigureAwait(false);


            // assert
            service.Received(dop).GetService();
        }
    }
}

Only the first test UsingAutoConfiguredNSubstituteCustomization failing randomly like this:

Expected to receive exactly 32 calls matching:
    GetService()
Actually received 39 matching calls:
    GetService()
    GetService()
    ...

[ploeh]

Thank you for writing. Before we start investigating this issue in the AutoFixture code base, do we know whether or not NSubstitute itself is thread-safe?

[MichaelLogutov]

NSubstitute should be thread safe. One of the three tests I've posted shows
that using raw NSubstitute works correctly.

[MichaelLogutov]

Any progress on that? Do you need any additional info?

[mrinaldi]

This is a tricky one.

NSubstitute is indeed thread-safe concerning the "replay" phase. That means that as long as you setup everything before spinning up the threads, you're safe - and that makes sense, your tests shouldn't setup in parallel.

However, AutoConfiguredNSubstituteCustomization sets up the substitute once the methods are first called. Thus, it hits the non thread-safe part of NSubstitute and things become unpredictable.

You can reproduce that introducing a new method to your interface and setting it up inside the threads:

public interface IServiceA
{
    IServiceB GetService();
    IServiceB GetServiceI();
}

private static async Task TestAsync(int dop, IServiceA service)
{
    // arrange
    var start = new SemaphoreSlim(0, dop);
    var cts = new CancellationTokenSource(TimeSpan.FromSeconds(1));

    var tasks = Enumerable
        .Range(0, dop)
        .Select(_ => Task.Run
                            (async () =>
                            {
                                await start.WaitAsync(cts.Token).ConfigureAwait(false);
                                service.GetService();
                                service.GetServiceI().Returns(x => Substitute.For<IServiceB>());
                            },
                            cts.Token));


    // act
    start.Release(dop);
    await Task.WhenAll(tasks).ConfigureAwait(false);


    // assert
    service.Received(dop).GetService();
}

There are two options here:
1 - Leave it as is and state that it's not supported to use AutoConfiguredNSubstituteCustomization in concurrency environments.
2 - Change the AutoConfiguredNSubstituteCustomization code to use some internal stuff of NSubstitute to achieve the thread-safety. I was able to do that, it's very brittle though. I had to update NSubstitute to 1.7.1 and since the internals of NSubstitute don't follow SemVer, we might have it broken on the next minor version released.

There's a third option, actually. I could send a PR to NSubstitute to make it more extensible and we work from there. The downside is that, first, @dtchepak must accept the PR. Second, everyone would have to update NSubstitute to its lastest version.

Any thoughts?

[dtchepak]

Hi @mrinaldi, I'm always open to PRs. :) What's the change you are proposing?

However, AutoConfiguredNSubstituteCustomization sets up the substitute once the methods are first called.

Does this mean a hacky workaround for the issue @MichaelLogutov is facing is to call something on the service before calling TestAsync? Far from ideal I know, but might be a quick way to get things working while we sort out a fix.

[MichaelLogutov]

Interesting. Since I wasn't able to reproduce this error on Moq and AutoMoq (with the same 3 tests) I think that indeed it would be beneficial for both libraries to come up with PR (maybe some kind of lock). Our project has 3k tests and the more we add the more false negatives we're getting in TeamCity runner. The recursive autowiring returned values is the most important feature to us in AutoFixture since it allows us to greatly reduce setup boilerplate code.

[mrinaldi]

@dtchepak I don't think this workaround will work. Since there's no way to know if a method has been setup, I use a ICallHandler (they have access to the ISubstituteState) to access the CallResults and, if not setup, I set it up.
This is done every time a method is called using the When..Do.

I didn't think through on the changes needed yet. We have a few options here.
In my point of view, the best extension point for AutoConfiguredNSubstituteCustomization would be to be able to inject a IAutoValueProvider. That would require a change in NSubstitute to use the IAutoValueProvider for ref and out parameters though or it'd be a breaking change for AutoConfiguredNSubstituteCustomization. The upside for AutoConfiguredNSubstituteCustomization is that it would work even for generic and void methods and it would become a real extension to the NSubstitute, instead of a hack.

The alternative is to create a Route overload in the ICallRouter that receives both the Route and the ICall, instead of the current two-step behavior of setting the route with SetRoute and then calling the Route.
In fact, this is where the race condition happens. After I call the SetRoute, the method in another thread calls a method in the substitute, effectively using my route, and when I call the Route, the route is set to the default route already.

There are a few point we could extend as alternative, but we can discuss that in the NSubstitute repository.

The downside to a PR, as I said, is that we'd have to update our dependency to NSubstitute to the new version. @ploeh, what do you think? Is that a problem?

[ploeh]

Updating to a new version isn't a problem as long as it isn't a breaking change. As a general rule, I don't want to update dependencies if there's no reason to do it, but if it improves the feature set of the library, it's warranted.

[MichaelLogutov]

Hello. Any progress on that? Can I help with something?

[zvirja]

Should be fixed in v4. The test above no longer fails with a new integration approach introduced in #832.