Fix bug with generic state parameter caused by inconsistent use of grainClassName / genericArgument / genericInterface

[Maarten88]

After lots of debugging on an issue I have with a generic grain that uses storage, I think that this change fixes the issue. The genericArgument was used inconsistently, and sometimes contained the genericInterface value. Also the grainClass parameter did not include the genericArguments during setup of the state, causing the grainType parameter in the first ReadStateAsync to be incorrect.

I moved the location where the conversion was made from SetupActivationInstance() into catalog.GetOrCreateActivation(), passed genericArguments in some functions and fixed the naming.

I think this fixes several bugs, including #1579.

Please test thoroughly, this "works on my machine" but the change might do things that I don't really understand and this is my first pull request on orleans.

[dnfclas]

Hi @Maarten88, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@Maarten88, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[veikkoeeva]

@Maarten88 I'll cross-link so I remember to remove the fix I put straight into the storage provider at #1682. Very nice you went throgh the trouble to the bottom of this!

[sergeybykov]

There is no need for this change, is there?

[Maarten88]

I did that to make it possible to reference the csproj file from another solution, in another directory. I thought this would come handy for others trying to debug/reference orleans inside their own project.

It's not needed for the fix itself.

[sergeybykov]

LGTM minus the minor code cleanup comments. I fired a functional test run for just in case.

[veikkoeeva]

@Maarten88, @sergeybykov So, if I understand correctly, I wouldn't need d5cff4b#diff-bf7a979df10dd018220615846b992e2bR459 anymore after this gets in?

[Maarten88]

@veikkoeeva with this change, grainType will always have the full typename, including generic arguments, in both ReadStateAsync and WriteStateAsync, so you should not need that anymore. Removing it would result in a different keyvalue that your current code (the non-generic typeName).

I wonder, does your workaround work currently with generic grains? You still seem to calculate a hash over the unnormalized grainType, which, before this, was different between read and write in the case of generic grain. Also normalizing the grainType would give MyGrain<int> and MyGrain<string> the same key.

[veikkoeeva]

@Maarten88 You are right! I seriously overlooked that when I just looked the grain reference and got things mixed up in my mind while putting in something that allowed to carry on.

There is another thing with this too, namely having grains of type MyGrain<int> and MyGrain<string> and having a generic state, e.g.

public class MyGrain<T>: Grain<SomeState<int>>, IMyGrain<T>
[...]
var grain1 = GrainClient.GrainFactory.GetGrain<IMyGrain<int>>(0);
var grain2 = GrainClient.GrainFactory.GetGrain<IMyGrain<string>>(0);

will throw an AggregateException telling

"Error creating activation for MyGrain`1. Message NewPlacement Request S127.0.0.1:22222:205585603_cli/4c2c27a2@37bc2787->S127.0.0.1:22222:205585603_grn/81758359/00000000@62053eae #4: global::IMyGrain:GetSomeType()"}
and so forth.

One way to reproduce this is to go the StorageProviders sample and make both Person and PersonState generic and then try to create those two grains. Looks like a bug or a limitation.

[Maarten88]

@veikkoeeva @sergeybykov I have wondered many times in the past days why activation does all these state related things at all. Why was that not implemented by simply calling ReadStateAsync from OnActivateAsync in the base class for a PersistentGrain? The read-side of GrainStateStorageBridge is mostly unused the way the code works now.

[sergeybykov]

Thank you, @Maarten88! This was an embarrassing and non-trivial to root-cause bug.

[veikkoeeva]

@Maarten88, probably @sergeybykov too, I wonder if it is problematic to have the version numbers there. This is grainType snatched from one of the tests

UnitTests.Grains.AzureStorageGenericGrain`1[[System.Int32, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]

Wouldn't this mean that when the version of the assembly is updated, the saved stated is rendered inaccessible?

[Maarten88]

@veikkoeeva @sergeybykov yes, I think using FullName for the type still causes problems (like you say: compatibility when using assembly versioning, but also the key length in TableStorage for generic grains getting too long because the .FullName ends up inside the partition key). This generic stuff certainly is not done yet, there also are still several tests disabled.

I think the next improvement / fix here would be to use type.ToString() instead of type.FullName, which gives a generic type that looks much saner, in your example it gives UnitTests.Grains.AzureStorageGenericGrain```1[System.Int32] which is what I'd expect it to be. I did a short spike yesterday to find out how much impact this change would have, only to find out that there's more .FullType and type string parsing going on than I can test before my vacation.

[veikkoeeva]

To save @ReubenBond's time, here's a a suggestion from him

We could just use TypeUtils.GetParseableName(type, options) and get back the (optionally fully qualified) C#-parseable type name without assembly-qualifications.

[veikkoeeva]

@Maarten88 Ah, see the comment. That function might be the solution. Whatever it is, I think we should have a visible function for it.