Threading issue with CallRouter.

[ghost]

CallRouter is not thread-safe because it allows mutation between SetSate(...) and Route(...). See for example: public static T Received(...) in SubstituteExtensions:

var router = GetRouterForSubstitute(substitute);
router.SetRoute(...);
return substitute;

Another thread could call SetRoute(...) directly after it has been called within Received(...) which would lead to a current route mutation and therefore unexpected or erroneous behaviour. Consider the following scenario:

public interface ICallback {
    void FirstCall( int value );
    void LastCall( int value );
}

class Dispatcher {
    private static int VALUE = -1;
    public static void Dispatch( ICallback callback ) {
        Task.Factory.StartNew( () => {
            var val = Interlocked.Increment( ref VALUE );
            callback.FirstCall( val );
            callback.LastCall( val );
        } );
    }
}

public class FailureTest {
    private AutoResetEvent firstCall;
    private AutoResetEvent lastCall;
    private ICallback callback;

    [SetUp]
    public void BeginTest() {
        firstCall = new AutoResetEvent( false );
        lastCall = new AutoResetEvent( false );

        callback = Substitute.For<ICallback>();
        callback.WhenForAnyArgs(
            obj => obj.FirstCall( 0 ) ).Do( args => {
                firstCall.Set();
            } );
        callback.WhenForAnyArgs(
            obj => obj.LastCall( 0 ) ).Do( args => {
                lastCall.Set();
            } );
    }

    [Test]
    public void Test() {
        for ( int i = 0; i < 1000; ++i ) {
            Dispatcher.Dispatch( callback );

            Assert.IsTrue( firstCall.WaitOne( TimeSpan.FromSeconds( 10 ) ) );
            callback.Received( i + 1 ).FirstCall( Arg.Any<int>() );

            Assert.IsTrue( lastCall.WaitOne( TimeSpan.FromSeconds( 10 ) ) );
            callback.Received( i + 1 ).LastCall( Arg.Any<int>() );
        }
    }
}

This test will sometimes fail within the Dispatcher when calling LastCall(...); because it is using the same Route as callback.Received( i + 1 ). Easy to very if catching the ReceivedCallsException in CallRouter.Route() around return routeToUseForThisCall.Handle( call );

A possible fix would be to attach the call route to the currently issued call of the substitute and making the CallRoute class immutable. I know I could change my tests to work around this issue but I think this library should be robust and safe to use from any number of threads.

I would be happy to submit a pull-request but I wanted to hear your thoughts on this issue first before starting any work on a possible patch.

[dtchepak]

Thanks @TheSatoshiChiba,

I think this is ok. The rule for threading with NSubstitute is that a substitute must be configured on a single thread, but should be callable from multiple threads. AFAICT this only affects configuration?

[ghost]

What about assertions? What is the threading rule on those? Because this issue would only allow assertions (checking for received calls) on a single thread. So it doesn't only affect configuration. It affects scenarios where you would have calls on the substituted object in one thread while trying to assert them in another at roughly the same time. If the usage of the library is intended to be single threaded for setup and assertion then it should be clearly documented.

[dtchepak]

Yes assertions should also be check by a single thread at a time.

NSubstitute's syntax means some unavoidable race conditions, such as a call happening between sub.Received() and the .SomeCall() (and also between a call and a subsequent call to Returns). So I've always considered the "test" part of a substitute's behaviour (i.e. configuration and assertions) to be one thing we where we need to make synchronous, compared with the "production" part where they should be able to work sensibly if the production code calls them from different threads.

Good suggestion about documenting this. Do you think this should just be a "Threading" sub-page on http://nsubstitute.github.io/help.html ?

[ghost]

Yes a threading sub-page that outlines the model seems like a sensible idea. Thank you 👍

[dtchepak]

Hi @TheSatoshiChiba,
Added a quick note on threading. Do you think that is sufficient? All suggestions welcome. :)

[ghost]

Looks good to me. Thank you :)