Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.NET Hot Reload breaks mocked interfaces and throws IndexOutOfRangeException #1252

Closed
frenzibyte opened this issue Apr 19, 2022 · 14 comments
Closed

Comments

@frenzibyte
Copy link

frenzibyte commented Apr 19, 2022

Hot reloading with interface mocks results in an internal failure once calling the method of the mock object.

Repro

There are two scenarios in which I can successfully reproduce this:

  1. Via calling mocked method after hot reload:

    var mock1 = new Mock<IInterface>();
    mock1.Setup(m => m.Method()).Callback(() => { });
    
    Console.WriteLine("Hot reload here.");
    Console.ReadLine();
    
    mock1.Object.Method();
    
    public interface IInterface
    {
        void Method();
    }
  2. Via calling mocked method before hot reload, then creating another mock and calling the method afterwards (note that if the method was not called before hot reload, then the issue does not occur):

    var mock1 = new Mock<IInterface>();
    mock1.Setup(m => m.Method()).Callback(() => { });
    
    mock1.Object.Method();
    
    Console.WriteLine("Hot broke here.");
    Console.ReadLine();
    
    var mock2 = new Mock<IInterface>();
    mock2.Setup(m => m.Method()).Callback(() => { });
    
    mock2.Object.Method();
    
    public interface IInterface
    {
        void Method();
    }

Expected

Calling Method() works just fine and no exception is thrown.

Actual

IndexOutOfRangeException at the point of calling Method() after hot reload:

Hot reload here.

Unhandled exception. System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at Moq.Extensions.GetImplementingMethod(MethodInfo method, Type proxyType) in C:\projects\moq4\src\Moq\Extensions.cs:line 150
   at Moq.Invocation.get_MethodImplementation() in C:\projects\moq4\src\Moq\Invocation.cs:line 52
   at Moq.MethodExpectation.IsOverride(Invocation invocation) in C:\projects\moq4\src\Moq\MethodExpectation.cs:line 183
   at Moq.MethodExpectation.IsMatch(Invocation invocation) in C:\projects\moq4\src\Moq\MethodExpectation.cs:line 125
   at Moq.Setup.Matches(Invocation invocation) in C:\projects\moq4\src\Moq\Setup.cs:line 104
   at Moq.FindAndExecuteMatchingSetup.<>c__DisplayClass0_0.<Handle>b__0(Setup setup) in C:\projects\moq4\src\Moq\Interception\InterceptionAspects.cs:line 92
   at Moq.SetupCollection.FindLast(Func`2 predicate) in C:\projects\moq4\src\Moq\SetupCollection.cs:line 120
   at Moq.FindAndExecuteMatchingSetup.Handle(Invocation invocation, Mock mock) in C:\projects\moq4\src\Moq\Interception\InterceptionAspects.cs:line 92
   at Moq.Mock.Moq.IInterceptor.Intercept(Invocation invocation) in C:\projects\moq4\src\Moq\Interception\Mock.cs:line 17
   at Moq.CastleProxyFactory.Interceptor.Intercept(IInvocation underlying) in C:\projects\moq4\src\Moq\Interception\CastleProxyFactory.cs:line 107
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.Proxies.IInterfaceProxy.Method()
   at Program.<Main>$(String[] args)

Moq version used here is v4.17.2


I've initially noticed this while hot reloading on JetBrains Rider, but can also reproduce this with dotnet watch.

@frenzibyte frenzibyte changed the title .NET Hot Reload breaks mocked interfaces .NET Hot Reload breaks mocked interfaces and throws IndexOutOfRangeException Apr 19, 2022
@stakx
Copy link
Contributor

stakx commented Apr 19, 2022

My initial suspicion is that Hot Reload / EnC does not mix well with the System.Reflection.Emit-powered proxy generation that happens under Moq's hood; esp. your second scenario. The first scenario we might get to work, albeit with a performance penalty for all other, more common use cases, so even that may not be worth doing.

@frenzibyte
Copy link
Author

I see, figured as much – Would you be able to provide any workaround to this issue? The only way I can see forward is to replace the interface with a class (as classes work for some odd reason), but that's not quite possible for our actual use case of moq.

@stakx
Copy link
Contributor

stakx commented Apr 19, 2022

Would you be able to provide any workaround to this issue?

Nope, and I'm not even going to try before having analyzed this in more detail. Don't use Hot Reload / EnC in combination with runtime-generated mocks?

@peppy
Copy link

peppy commented Apr 20, 2022

Don't use Hot Reload / EnC in combination with runtime-generated mocks

From this response it sounds like you believe a use case for this is quite unimaginable (though I may be reading into it wrong), so I wanted to provide a bit of insight that may or may not be helpful.

This issue was found when beginning to use Moq in a larger way in our game engine's testing framework (@frenzibyte helps with our project). We have our testing framework setup in a way that allows them to either run headless via NUnit (quite standard) but also via a visual explorer, allowing for immediate feedback during implementation.

Until now we have created subclasses to isolate dependencies in test, but are hoping to make more liberal use of Moq. It works great for classes, but as soon as we started to migrate components to interfaces we hit the issue (which is a serious blocker for us).

Example of the code which causes the issue: https://github.com/ppy/osu/pull/17881/files#diff-e24429f0b2e7c48d14874361e6c6ad8cc56340510e20ba6cf830f6213acd6e94R48-R55

Example of how our testing framework works and why we would hope that hot reload could be used successfully:

JetBrains.Rider.2022-04-20.at.03.06.19.mp4

What currently happens with moq:

JetBrains.Rider.2022-04-20.at.03.08.13-converted.3.mp4

Thanks for your consideration 🙏

@stakx
Copy link
Contributor

stakx commented Apr 20, 2022

From this response it sounds like you believe a use case for this is quite unimaginable

That wasn't my intention at all. Apologies if my suggestion came across as somewhat snappy. It was meant as my estimation of what you might realistically have to resort to in the end. I will look into this a little more closely, but knowing the internals of Moq, DynamicProxy, and a bit about how the CLR handles metadata & EnC, I don't expect that there will be good news for you.

Thanks for adding some more background, that's always helpful!

@stakx
Copy link
Contributor

stakx commented May 17, 2022

I'm currently looking into this. At first glance, it appears as if MethodInfo.Equals were broken. We've got two MethodInfos for the same method (they refer to the same Module and MetadataToken), yet testing for equality claims that they're different.

methodinfo-comparison

Unless I am missing something obvious, we might have to report that to the .NET runtime folks. I'll see if I can derive simpler repro code that doesn't rely on Moq / DynamicProxy.

@stakx
Copy link
Contributor

stakx commented May 17, 2022

I've reported this over at dotnet/runtime#69427.

@stakx
Copy link
Contributor

stakx commented May 20, 2022

The above report was marked as a duplicate, the issue is now being tracked under dotnet/runtime#67989.

Nope, back to a separate issue. 😆

@stakx
Copy link
Contributor

stakx commented Jul 25, 2022

@peppy, it just ocurred to me that you may be able to avoid this error with Hot Reload by downgrading Moq. Moq didn't always cache those MethodInfo that cause trouble here, so earlier versions might work. (I don't know without checking the source code how many minor versions you need to go back; I'm assuming 4.7 as the lower boundary.)

@peppy
Copy link

peppy commented Aug 1, 2022

Thanks for the suggestion, but it seems like going that far back causes package installation fails against net6.0:

[Notification][Install] Install failed (project: osu.Game.Tests, package: Moq v4.5.30)
Package restore failed. Rolling back package changes for 'osu.Game.Tests'.
Package 'Castle.Core 3.3.3' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8 ... red using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.

@stakx
Copy link
Contributor

stakx commented Aug 1, 2022

@peppy, I see. If you had to go back further than where we introduced .NET Standard as a TFM, you'd be out of luck. Too bad.

@peppy
Copy link

peppy commented Aug 5, 2022

I think I may have misunderstood what you meant by lower bound, and went straight to that point in time. I'll do some further testing with versions in between soon.

@stakx
Copy link
Contributor

stakx commented Dec 12, 2022

@peppy, it seems that this runtime bug has since been fixed. I'm not sure if the patch was merged in time to make it into .NET 7 or not. Could you perhaps give this a quick try (using the latest available .NET runtime version) and report back whether this issue has been resolved?

@frenzibyte
Copy link
Author

I can confirm this issue has been fixed on .NET SDK 7.0.101! Thank you very much.

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

No branches or pull requests

3 participants