4.1 does not support multi-threading

[DanTup]

It seems the "fix" for some race conditions in 4.0 was to put locks around mocks internally. This breaks any tests that were using mocks in a multi-threaded way (we've had to roll back to 4.0 for our concurrency tests to work).

If the default is going to be that a mock is not thread-safe, there should at least be some way of opting in to a thread-safe mock. Despite the other issues raised in 4.0, our tests were 100% reliable; but in 4.1 they're 100% broken; so this seems to be a big breaking change in terms of functionality 👎

[MatKubicki]

I agree with DanTup, this is a real pain as we have tests that use threads to test a wrapper around something is none blocking, this something is mocked and the test is now failing as the mock itself is doing the blocking.

[kzu]

Would be great if you guys could put together a PR with a fix that satisfies both bug reports: the one that the mocks break when accessed from multiple threads, and the one where you want to actually lock outside.

Maybe there's a new property on the mock like new Mock { ThreadSafe = false } or the like?

[MatKubicki]

Making the lock optional would be a very easy fix, and we can keep using the existing tests for issue 249 and added the reverse test for this fix.

But. It does seem like an easy way out! Reading the original bug on Google Code (http://code.google.com/p/moq/issues/detail?id=249) it seems that the problems were all caused by the concurrent access to the ActualInvocations in the AddActualInvocation interception strategy. Adding a lock specifically to that fixes all the repo cases I've found.

Looking around the code it seems that there are other things in the interceptors that should be locked, mostly on the InterceptStrategyContext object.

SO my question is, do we want to add the ThreadSafe option to the mock, which essentially blocks mutlithreaded tests unless we enable it and accept that it has a potential to crash. OR do you think trying to sort out the underlying problem is a better course of action? I'm unsure as this is my first time looking at Moq's code base.

[kzu]

In all honesty, I thought the previous PR had fixed the issue for good.
Turns out that it apparently didn't.

So I'm all for making it work as it should, and support both scenarios
seamessly.

I'm very greateful for taking a closer look at this issue. I've been lucky
enough not to have to do multithreaded tests/mocks, but this issue has been
on the backlog for way too long already :(

Thanks in advance for any effort you can put on it,

/kzu

/kzu

Daniel Cazzulino

On Tue, Dec 10, 2013 at 7:06 AM, MatKubicki notifications@github.comwrote:

Making the lock optional would be a very easy fix, and we can keep using
the existing tests for issue 249 and added the reverse test for this fix.

But. It does seem like an easy way out! Reading the original bug on Google
Code (http://code.google.com/p/moq/issues/detail?id=249) it seems that
the problems were all caused by the concurrent access to the
ActualInvocations in the AddActualInvocation interception strategy. Adding
a lock specifically to that fixes all the repo cases I've found.

Looking around the code it seems that there are other things in the
interceptors that should be locked, mostly on the InterceptStrategyContext
object.

SO my question is, do we want to add the ThreadSafe option to the mock,
which essentially blocks mutlithreaded tests unless we enable it and accept
that it has a potential to crash. OR do you think trying to sort out the
underlying problem is a better course of action? I'm unsure as this is my
first looking at Moq's code base.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/62#issuecomment-30212968
.

[MatKubicki]

The fix for this issue also fixes issue #47.

I have used the test case from issue 47, with a bit of modification, as a test case for my new code. I have removed the test case added for issue 249 as that was explicitly checking the reverse case, that mocks we're not multi-threaded.

All I need to do now is work out how to commit this code to GitHub without it reporting every file as changed. I imagined it was a line feed/ carriage return issue but the files that I have changed appear fine. I am trying to use the core.autocrlf option set to true, but the behavior seemed the same with it set to false as well.

I may just give up and make my commits via the website!

[MatKubicki]

Ok i've added a pull request #68. Hope I've done this right, if not then please let me know where i've gone wrong, I'm new to Git and Github!

[DanTup]

Sorry for the lack of responses; taken me a while to catch up!

I've had a quick scan through the PR; there's a lot of changes in there - would using a ConcurrentDictionary not be better than all the locks for invocationLists? (assuming you're building in a version of .NET that contains them)

(Added this comment to the PR; guess it makes more sense there; though don't mistake me for someone of authority, I'm just a user!)

[MatKubicki]

I think Moq is planning to continue support for 3.5, I certainly hope it is as we're stuck with it here for a while longer! I did start writing with the ConcurrentDictionary at first, but reverted once I remembered that.

[DanTup]

Ah, makes sense. Doh!

[kzu]

I scanned the PR and it looks good. I like the cleanup you've done too,
separates the responsibilities better.

I'd like to get more feedback on the fix so that we all agree this is the
one that will fix it for good and supports all required scenarios :)

Looping moqdisc for that purpose

/kzu

Daniel Cazzulino

On Wed, Dec 11, 2013 at 11:00 AM, MatKubicki notifications@github.comwrote:

I think Moq is planning to continue support for 3.5, I certainly hope it
is as we're stuck with it here for a while longer! I did start writing with
the ConcurrentDictionary at first, but reverted once I remembered that.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/62#issuecomment-30322049
.

[DanTup]

I can try and run our tests with it; that reliably fail in the latest release (we had to roll back); but it's not likely to be this week due to some fire-fighting :(

[JeffAtDeere]

I've got another example of Moq failing when run parallel. I finally managed to simplify a flaky test down to the following code. When run in single-threaded mode (1 degree of parallelism), it works. Anything more [on a multi-core machine] and it fails. I've cloned the Moq source and rebuilt as of today.

[TestClass]
public class ParallelTest
{
    [TestMethod]
    public void CheckParallel()
    {
        var dataObjects = new[] { 1, 2 };

        var mock = new Mock<IWorker>();
        mock.Setup(x => x.AddTwo(1)).Returns(3);
        mock.Setup(x => x.AddTwo(2)).Returns(4);

        var main = new Main(mock.Object);
        main.DoWork(dataObjects);

        mock.Verify(x => x.SaveValue(3));
        mock.Verify(x => x.SaveValue(4));
    }
}

public class Main
{
    private readonly IWorker _worker;

    public Main(IWorker worker)
    {
        _worker = worker;
    }

    public void DoWork(IEnumerable<int> objects)
    {
        objects.AsParallel().WithDegreeOfParallelism(4).ForAll(Compute);
    }

    private void Compute(int i)
    {
        var result = _worker.AddTwo(i);
        Console.WriteLine(result);
        _worker.SaveValue(result);
    }
}

public interface IWorker
{
    int AddTwo(int i);
    void SaveValue(int result);
}

[MatKubicki]

Of by fail you mean fails the test them that would make sense as moq will be returning the wrong value from AddTwo when run in parallel.

Is be interested to know if this fails with my PR, I would guess not. I'll try for myself on my return to the office on Thursday.

[kzu]

That would be awesome Matthew!

/kzu from mobile
On Dec 31, 2013 8:51 PM, "Matthew Kubicki" notifications@github.com wrote:

Of by fail you mean fails the test them that would make sense as moq will
be returning the wrong value from AddTwo when run in parallel.

Is be interested to know if this fails with my PR, I would guess not. I'll
try for myself on my return to the office on Thursday.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/62#issuecomment-31415454
.

[MatKubicki]

This is now fixed with my PR that i've added for #78, I seem to have mucked up somewhere when it comes to adding that PR as it has instead appeared as its own issue, #80. Either way the latest code from @JeffAtDeere is pretty much the same as the test case from #78 and both are fixed by #80's PR!

On second glance I see that its fine, and just my lack of familiarity with Github, the PR #80 fixes all.

[DanTup]

Apologies for the delay... I tested both the latest NuGet package (which had the first set of changes in) and the latest code from @MatKubicki's repo, and both versions result in our concurrency tests passing (which were previously failing due to the locks causing them to run one after the other).

I haven't reviewed the code; but it definitely seems to fix the regression I was reporting in this case 👍

[kzu]

Great. Will ship a new version today

/kzu from mobile
On Jan 3, 2014 7:46 PM, "Danny Tuppeny" notifications@github.com wrote:

Apologies for the delay... I tested both the latest NuGet package (which
had the first set of changes in) and the latest code from @MatKubickihttps://github.com/MatKubicki's
repo, and both versions result in our concurrency tests passing (which were
previously failing due to the locks causing them to run one after the
other).

I haven't reviewed the code; but it definitely seems to fix the regression
I was reporting in this case [image: 👍]

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/62#issuecomment-31560323
.