-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix bug with generic state parameter caused by inconsistent use of grainClassName / genericArgument / genericInterface #1897
Conversation
Hi @Maarten88, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
@Maarten88, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
@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! |
@@ -422,8 +422,8 @@ | |||
<Message Text="[OrleansDllBootstrapUsingCodeGen] - Compiling Orleans.dll for bootstrap" Importance="high" /> | |||
<MSBuild Projects="$(MSBuildProjectFullPath)" Targets="Build" Properties="Bootstrap=true;BootstrapOutputPath=$(BootstrapOutputPath);DefineConstants=$(ExcludeCodeGen)" UnloadProjectsOnCompletion="true" UseResultsCache="false" /> | |||
<Message Text="[OrleansDllBootstrapUsingCodeGen] - Compiling code generators for bootstrap" Importance="high" /> | |||
<MSBuild Projects="$(SolutionDir)\OrleansCodeGenerator\OrleansCodeGenerator.csproj" Targets="Build" Properties="Bootstrap=true;BootstrapOutputPath=$(BootstrapOutputPath);OutputPath=$(BootstrapOutputPath);OutDir=$(BootstrapOutputPath);DefineConstants=$(ExcludeCodeGen)" UnloadProjectsOnCompletion="true" UseResultsCache="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for this change, is there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
LGTM minus the minor code cleanup comments. I fired a functional test run for just in case. |
…nericArgs translation deeper into GrainFactoryBase. Enabled testcase for dotnet#1579
4bd7ae0
to
f634bde
Compare
@Maarten88, @sergeybykov So, if I understand correctly, I wouldn't need d5cff4b#diff-bf7a979df10dd018220615846b992e2bR459 anymore after this gets in? |
@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 |
@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
will throw an
One way to reproduce this is to go the StorageProviders sample and make both |
@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. |
Thank you, @Maarten88! This was an embarrassing and non-trivial to root-cause bug. |
@Maarten88, probably @sergeybykov too, I wonder if it is problematic to have the version numbers there. This is
Wouldn't this mean that when the version of the assembly is updated, the saved stated is rendered inaccessible? |
@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 |
To save @ReubenBond's time, here's a a suggestion from him
|
@Maarten88 Ah, see the comment. That function might be the solution. Whatever it is, I think we should have a visible function for it. |
By doing this, I encountered that this is still failing on the call to ClearState
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.