Overloaded generic grain methods resolve to only 1 implementation

[EdeMeijer]

I had some weird issue and boiled it down to the following. When a grain has 2 generic methods with the same name but different arguments, then no matter the arguments you pass, it will always dispatch to the same implementation.

Say we have the following grain

public interface IFoo : IGrainWithGuidKey
{
    Task<int> Act<T>(int a);

    Task<int> Act<T>(int a, string b);
}

public class FooGrain : Grain, IFoo
{
    public Task<int> Act<T>(int a) => Task.FromResult(1);

    public Task<int> Act<T>(int a, string b) => Task.FromResult(2);
}

Then, when we call the first version with a single argument, it correctly dispatches to the first implementation:

var sut = GrainFactory.GetGrain<IFoo>(Guid.NewGuid());
var res = await sut.Act<bool>(42);
Assert.Equal(1, res); // Works

However, when we call the other overload, it in fact dispatches to the same implementation as above:

var sut = GrainFactory.GetGrain<IFoo>(Guid.NewGuid());
var res = await sut.Act<bool>(42, "a");
Assert.Equal(2, res); // Fails, got 1

When the generic type argument is removed from the methods, everything works just fine. If I rename one of the overloads everything works fine as well (this is my workaround for now in our project). It seems to have something to do with GenericMethodInvoker, which just creates 1 invoker function per grain-method name combination regardless of signature.

[oising]

@EdeMeijer -- has it anything to do with the fact that your generic type parameter, T is not used? It might be triggering a buggy code path / optimisation in the codegen...

[oising]

I cannot repro on orleans 3.3.0 -- what version are you using? Also, what codegenerator? the .Build or .MSBuild version? The latter one is the newest.

[EdeMeijer]

I am on 3.3.0 as well, and in the actual case where I encountered the issue, the generic type param was definitely used. Also, I'm using the .MSBuild version as well. Weird that you cannot reproduce it. I'll see if I can make a little self-contained project that you can run to reproduce it.

[EdeMeijer]

https://github.com/EdeMeijer/orleans-bug-8601-repro

Just dotnet run that and you should see:

Expected: 1, actual: 1
Expected: 2, actual: 1

[oising]

Ok -- I see the same thing. I don't what the hell I did last time. Clearly I messed up something silly.

[oising]

Ok, I had a look at the code generator and it's broken, but I see the problem. It's always binding to the first overload of the targeted generic method. It doesn't even try to take the method parameters into account when binding... ouch. I'm hacking on a PR.

[EdeMeijer]

Good to hear, and that's exactly what I thought when quickly looking into the generator code, but I'm not very familiar with it so I wasn't sure.

[oising]

I tested the first time with orleanstestkit -- but of course that doesn't go through the generated proxies, so I didn't see the issue. Silly. The fix is actually looking more complicated than I initially thought. I can make it work easily for multiple overloads, as long as none of the parameters are generic (which is a highly unlikely scenario.) The reason for the complexity is that Orleans is pinned still to the .net framework, and the reflection infra is severely lacking for working with generics. In .net core, it's easy to manage -- I'm still pushing ahead though.

[oising]

Hmm, seems this has put an end to my efforts: https://github.com/dotnet/runtime/issues/20377

TL;DR -- Orleans will need to move to netstandard / dotnetcore for this to be feasible.

[EdeMeijer]

Hmm that's too bad. Still, the current behaviour is very confusing and unsound. Would it be possible to detect when multiple overloads of a generic method are used in a grain interface, and just raise an error during compile time instead? Just explain why that feature is not supported and suggest to use different method names and this would be less of an issue.

[oising]

Hey @EdeMeijer -- that's what I'm suggesting also to Microsoft, although at runtime. I'm not sure how hard it would be to implement compile-time detection, but runtime detection is considerably easier. It's better than nothing...

[oising]

@EdeMeijer Good news -- I spent some more time digging into the invoker and I realized that I have enough information from the context to make it work. Orleans gives me the actual type parameters, not just the arity, so I'm able to close the generic method definition and compare the parameter types for compatibility without worrying about generic placeholder types. I'm writing unit tests to make sure I cover all the cases (covariant types like arrays etc, unused generic type param, multiple type params etc.) I'll link this issue in the PR when I submit it.

Here are some basic unit tests passing:

[oising]

Okay... so, this mostly works. Unfortunately it has uncovered more deficiencies further upstream in Orleans. If any of the runtime arguments are null, matching against overloads becomes ambiguous because the invocation is dynamic. Only the values are given, not the types of the passed arguments. I'm digging further...

[EdeMeijer]

Good to see you're working on it, I'm sure you can figure out something to tackle these last issues

[oising]

I think I cracked it tonight:

[EdeMeijer]

Oh that's great! Looks like you put a lot of effort into this, just so I can give two methods the same name again :)

[oising]

Well, that and it seems to be a reasonably big "gotcha" that is potentially hiding buggy code. And in this lockdown I needed something to focus on other than work! I probably only put about 10 hours into it, most of which was trying to grok the codegen/invoker :)

[EdeMeijer]

Hey, I upgraded to Orleans 3.5.0 which includes this fix, but unfortunately ran into the next issue which seems very much related. The example code I provided now works, but as soon as the two methods use different generic type constraint, it breaks down with a type constraint violation error.

Example code:

public interface IFoo : IGrainWithGuidKey
{
    Task<int> Act<T>(int a) where T : IAnimal;

    Task<int> Act<T>(int a, string b) where T : ICar;
}

public class FooGrain : Grain, IFoo
{
    public Task<int> Act<T>(int a) where T : IAnimal => Task.FromResult(1);

    public Task<int> Act<T>(int a, string b) where T : ICar => Task.FromResult(2);
}

public interface IAnimal
{
}

public class Cat : IAnimal
{
}

public interface ICar
{
}

public class Toyota : ICar
{
}

var sut = GrainFactory.GetGrain<IFoo>(Guid.NewGuid());
var res = await sut.Act<Cat>(42);
Assert.Equal(1, res); // Works

res = await sut.Act<Toyota>(42, "a"); // Throws Act[T](Int32)' violates the constraint of type 'T'
Assert.Equal(2, res);

Output (when running from a unit test, slightly redacted)

System.ArgumentException
GenericArguments[0], '****.Tests.Orleans.GenericArgumentTest+Toyota', on 'System.Threading.Tasks.Task`1[System.Int32] Act[T](Int32)' violates the constraint of type 'T'.
   at System.RuntimeType.ValidateGenericArguments(MemberInfo definition, RuntimeType[] genericArguments, Exception e)
   at System.Reflection.RuntimeMethodInfo.MakeGenericMethod(Type[] methodInstantiation)
   at Orleans.CodeGeneration.GenericMethodInvoker.GetMethod(Type declaringType, String methodName, Type[] typeParameters, Type[] parameterTypes)
   at Orleans.CodeGeneration.GenericMethodInvoker.CreateInvoker(Object[] arguments)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Orleans.CodeGeneration.GenericMethodInvoker.Invoke(IAddressable grain, Object[] arguments)
   at ****.Tests.Orleans.OrleansCodeGenFooMethodInvoker.Invoke(IAddressable grain, InvokeMethodRequest request) in /home/ede/repos/****/****/Tests/obj/local/Debug/net5.0/Tests.orleans.g.cs:line 1439
   at Orleans.Runtime.GrainMethodInvoker.Invoke()
   at Orleans.Runtime.InsideRuntimeClient.Invoke(IAddressable target, IInvokable invokable, Message message)
   at Orleans.Internal.OrleansTaskExtentions.<ToTypedTask>g__ConvertAsync|4_0[T](Task`1 asyncTask)
   at ****.Tests.Orleans.GenericArgumentTest.TestGenericOverloads() in /home/ede/repos/****/****/Tests/Orleans/GenericArgumentTest.cs:line 22

I'm not sure what's going on, but it seems it somehow tries to invoke the overload with 1 argument again.

Could you please have a look at this again? The actual use case is that 1 method has a type constraint on IGrainWithGuidKey and accepts a Guid parameter, and the other has a type constraint on IGrainWithGuidCompoundKey and accepts a Guid + string argument.

[oising]

Hi @EdeMeijer -- can you open a new ticket for this one please? I can see how that would be a problem with the current implementation but I'm not sure how much work it would be to add this. Because I had to write a generic method overload resolver myself (as this is dynamic dispatch at runtime, not compile time), it is a bit limited. I'll look into it, but I can't promise anything :/ Once you get into working with constraints, we'd have to test against interface constraints (what you're doing) but also reference constraints (class), nullable (notnull) and also default ctor (new()), edit: oh, also enum is a recent addition too, right? edit2: oh lord, variance is another consideration. All of which add to overhead when dispatching the call, and every time the call happens.

I don't think Orleans 4.0 will have this problem, but @ReubenBond should confirm this.

[oising]

I think not adding a compile time check in the codegenerator that warns about not supporting constraints was an oversight on my behalf.

[ReubenBond]

vNext doesn't have this issue, but it should be fixable for 3.x, too, since there are a different number of parameters in this case. Note that .NET doesn't support overloading a method/type by varying only constraints

[oising]

vNext doesn't have this issue, but it should be fixable for 3.x, too, since there are a different number of parameters in this case. Note that .NET doesn't support overloading a method/type by varying only constraints

That's not what I meant by "variance" - I was thinking about co/contravariant interface constraints - but incidentally you're also unfortunately [partially] wrong with this. The following code is legitimate c#:

class Foo
{
    public virtual void Method<T>(T t) where T : class { }
    public virtual void Method<T>(T t) where T : struct { }
}

...and is why the default constraint was added in c# 9 to allow overriding of the above methods -- the overridden methods in a subclass must use where T : default to allow to compiler to do the work of figuring out the correct "inherited" constraint.

[ReubenBond]

[oising]

Oops, I was pulling from memory -- the parameters have to be nullable:

class Foo
{
    public virtual void Method<T>(T? t) where T : class { }
    public virtual void Method<T>(T? t) where T : struct { }
}

Found docs: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/unconstrained-type-parameter-annotations#default-constraint

I don't think my explanation in previous post was correct about default either -- it seems more complicated than that. Weird edge cases...

[ReubenBond]

That's an odd case, but the parameter types in that case are very different, since the struct variant is actually Nullable<T> and the non-struct variant is a regular T with (essentially) an attribute/metadata which doesn't alter the signature, so the interface is equivalent to the following as far as signatures are concerned:

class Foo
{
    public virtual void Method<T>(T t) where T : class { }
    public virtual void Method<T>(Nullable<T> t) where T : struct { }
}

[oising]

Yep, you're totally right -- still, you can see why my spider sense was tingling :D

[EdeMeijer]

I think you're overthinking it, since like Reuben says, there's no overloading on type constraints. Just match the number of parameters and their types and you should be good, right?

Anyhow, I'll make a new issue for you this Monday.