Deadlock in Interceptor #47

Closed
asmus opened this Issue Apr 24, 2013 · 9 comments

Comments

Projects
None yet
6 participants
@asmus

asmus commented Apr 24, 2013

When calling Method1 on the mock, and Method one is executing Method2 asynchronously and then waiting for Method2 to complete Moq will cause a deadlock due to the interceptor implementation.

public void Intercept(ICallContext invocation)
{

    lock (Mock) // this solves issue #249
    {
        var interceptionContext = new InterceptStrategyContext(Mock
                                                            , targetType
                                                            , invocationLists
                                                            , actualInvocations
                                                            , behavior
                                                            , orderedCalls
                                                            );
        foreach (var strategy in InterceptionStrategies())
        {
            if (InterceptionAction.Stop == strategy.HandleIntercept(invocation, interceptionContext))
            {
                break;
            }
        }
    }
}

The actual method of the Mock will be called unter lock condition. This will cause a deadlock in some cases. Simple TestCase to reproduce the behavior:

public class ClassToMock
{
    AutoResetEvent reset = new AutoResetEvent(false);
    public virtual void M1()
    {
        var task = new TaskFactory().StartNew(M2);
        Thread.Sleep(500);
        reset.Set();
        task.Wait();
    }

    public virtual void M2()
    {
        reset.WaitOne();
    }
}

[TestMethod]
public void TestMock()
{
    var testMock = new Mock<ClassToMock>{CallBase = true};
    testMock.Object.M1(); // <-- This will never return!
    testMock.Verify(x => x.M1());
    testMock.Verify(x => x.M2());
}
@kzu

This comment has been minimized.

Show comment
Hide comment
@kzu

kzu Apr 24, 2013

Member

Could this be related to the fix @yonahw contributed in pull #36 ?

Member

kzu commented Apr 24, 2013

Could this be related to the fix @yonahw contributed in pull #36 ?

@asmus

This comment has been minimized.

Show comment
Hide comment
@asmus

asmus Apr 24, 2013

I don´t think so. The InvokeBase Interception strategy is still called under lock conditions and the lock is still made on "Mock". I believe executing the InvokeBase strategy under lock conditions is causing the deadlock. Unfortunately I do not have the time to verify that right now. Maybe I can can try to fix it on the weekend.

asmus commented Apr 24, 2013

I don´t think so. The InvokeBase Interception strategy is still called under lock conditions and the lock is still made on "Mock". I believe executing the InvokeBase strategy under lock conditions is causing the deadlock. Unfortunately I do not have the time to verify that right now. Maybe I can can try to fix it on the weekend.

@yonahw

This comment has been minimized.

Show comment
Hide comment
@yonahw

yonahw Apr 25, 2013

Contributor

I didn't have time to run test myself but doesn't seem related on the surface.

Contributor

yonahw commented Apr 25, 2013

I didn't have time to run test myself but doesn't seem related on the surface.

@kzu

This comment has been minimized.

Show comment
Hide comment
@kzu

kzu Apr 25, 2013

Member

weird thing is that there was no other change related to locking, so I
wonder how could the behavior change so drastically (do we know it's
different than before?)

/kzu

Daniel Cazzulino

On Thu, Apr 25, 2013 at 10:02 AM, Yonah Wahrhaftig <notifications@github.com

wrote:

I didn't have time to run test myself but doesn't seem related on the
surface.


Reply to this email directly or view it on GitHubhttps://github.com/Moq/moq4/issues/47#issuecomment-17005259
.

Member

kzu commented Apr 25, 2013

weird thing is that there was no other change related to locking, so I
wonder how could the behavior change so drastically (do we know it's
different than before?)

/kzu

Daniel Cazzulino

On Thu, Apr 25, 2013 at 10:02 AM, Yonah Wahrhaftig <notifications@github.com

wrote:

I didn't have time to run test myself but doesn't seem related on the
surface.


Reply to this email directly or view it on GitHubhttps://github.com/Moq/moq4/issues/47#issuecomment-17005259
.

@asmus

This comment has been minimized.

Show comment
Hide comment
@asmus

asmus Apr 25, 2013

In my opinion this behavior was introduced when fixing issue #3. As far as I can tell (I´m still not so familiar with github) the lock was introduced to resolve the threading issue when calling methods on a mock from different threads in parallel. The lock is just covering to much. It is calling "foreign" code under lock conditions, what always is dangerous. But on the other hand it could very well be that I´m missing something here and I´m completely wrong .... that happens from time to time :)

asmus commented Apr 25, 2013

In my opinion this behavior was introduced when fixing issue #3. As far as I can tell (I´m still not so familiar with github) the lock was introduced to resolve the threading issue when calling methods on a mock from different threads in parallel. The lock is just covering to much. It is calling "foreign" code under lock conditions, what always is dangerous. But on the other hand it could very well be that I´m missing something here and I´m completely wrong .... that happens from time to time :)

@kzu

This comment has been minimized.

Show comment
Hide comment
@kzu

kzu May 27, 2013

Member

@FelicePollano did you have a chance to look at this?

Member

kzu commented May 27, 2013

@FelicePollano did you have a chance to look at this?

@FelicePollano

This comment has been minimized.

Show comment
Hide comment
@FelicePollano

FelicePollano May 27, 2013

Contributor

Full immersion until the end of the month.

 

Felice Pollano
Senior IT Architect & Developer

http://www.felicepollano.com

From: "Daniel Cazzulino" notifications@github.com
To: "Moq/moq4" moq4@noreply.github.com
Cc: "Felice Pollano" felice@felicepollano.com
Date: Mon, 27 May 2013 08:17:33 -0700
Subject: Re: [moq4] Deadlock in Interceptor (#47)

@FelicePollano did you have a chance to look at this?

—> Reply to this email directly or view it on GitHub.

Contributor

FelicePollano commented May 27, 2013

Full immersion until the end of the month.

 

Felice Pollano
Senior IT Architect & Developer

http://www.felicepollano.com

From: "Daniel Cazzulino" notifications@github.com
To: "Moq/moq4" moq4@noreply.github.com
Cc: "Felice Pollano" felice@felicepollano.com
Date: Mon, 27 May 2013 08:17:33 -0700
Subject: Re: [moq4] Deadlock in Interceptor (#47)

@FelicePollano did you have a chance to look at this?

—> Reply to this email directly or view it on GitHub.

@asmus

This comment has been minimized.

Show comment
Hide comment
@asmus

asmus May 29, 2013

I thought I have a look at the code and try to fix it. Got the code from gitHub and build the whole thing (VS 2012). But somehow almost all UnitTest are failing with:

System.TypeInitializationException
The type initializer for 'Moq.Mock1' threw an exception. at Moq.Mock1.b__0() in Mock.Generic.cs: line 138
at Moq.PexProtector.Invoke(Action action) in PexProtector.cs: line 56
at Moq.Mock1.InitializeInstance() in Mock.Generic.cs: line 136 at Moq.Mock1.OnGetObject() in Mock.Generic.cs: line 153
at Moq.Mock.GetObject() in Mock.cs: line 152
at Moq.Mock.get_Object() in Mock.cs: line 147
at Moq.Mock1.get_Object() in Mock.Generic.cs: line 131 at Moq.Tests.AsInterfaceFixture.GetMockFromNonAddedInterfaceThrows() in AsInterfaceFixture.cs: line 120 System.TypeInitializationException The type initializer for 'Moq.Proxy.CastleProxyFactory' threw an exception. at Moq.Proxy.CastleProxyFactory..ctor() at Moq.Mock1..cctor() in Mock.Generic.cs: line 54
System.IO.FileLoadException
Could not load file or assembly 'Castle.Core, Version=2.5.0.0, Culture=neutral, PublicKeyToken=407dd0808d44fbdc' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
at Moq.Proxy.CastleProxyFactory..cctor()

Obviously I miss something here ... any hints?

asmus commented May 29, 2013

I thought I have a look at the code and try to fix it. Got the code from gitHub and build the whole thing (VS 2012). But somehow almost all UnitTest are failing with:

System.TypeInitializationException
The type initializer for 'Moq.Mock1' threw an exception. at Moq.Mock1.b__0() in Mock.Generic.cs: line 138
at Moq.PexProtector.Invoke(Action action) in PexProtector.cs: line 56
at Moq.Mock1.InitializeInstance() in Mock.Generic.cs: line 136 at Moq.Mock1.OnGetObject() in Mock.Generic.cs: line 153
at Moq.Mock.GetObject() in Mock.cs: line 152
at Moq.Mock.get_Object() in Mock.cs: line 147
at Moq.Mock1.get_Object() in Mock.Generic.cs: line 131 at Moq.Tests.AsInterfaceFixture.GetMockFromNonAddedInterfaceThrows() in AsInterfaceFixture.cs: line 120 System.TypeInitializationException The type initializer for 'Moq.Proxy.CastleProxyFactory' threw an exception. at Moq.Proxy.CastleProxyFactory..ctor() at Moq.Mock1..cctor() in Mock.Generic.cs: line 54
System.IO.FileLoadException
Could not load file or assembly 'Castle.Core, Version=2.5.0.0, Culture=neutral, PublicKeyToken=407dd0808d44fbdc' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
at Moq.Proxy.CastleProxyFactory..cctor()

Obviously I miss something here ... any hints?

@MatKubicki

This comment has been minimized.

Show comment
Hide comment
@MatKubicki

MatKubicki Dec 11, 2013

Contributor

I've added pull request Moq#68 that fixes this problem, i've used the code asmus attached as test for the fix.

Contributor

MatKubicki commented Dec 11, 2013

I've added pull request Moq#68 that fixes this problem, i've used the code asmus attached as test for the fix.

@stakx stakx closed this Jun 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment