Add TGrainState type constraints to document necessary system constraints - Resolves #1906 #1923

normanhh3 · 2016-07-09T00:01:43Z

This adds constraint checking to the type for State types so that implementers get compile time checks instead of runtime errors.

…ks implementing stateful grains. Fixes #1906

dnfclas · 2016-07-09T00:01:48Z

Hi @normanhh3, 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;

gabikliot · 2016-07-10T04:31:31Z

I think we can safely limit grain state to be a class. No need to support structs, right?

normanhh3 · 2016-07-10T13:09:40Z

Activator.CreateInstance seems to be the limiting factor. I don't believe structs are supported today in the code so adding this constraint to exclude them would not break existing functionality from my reading of the code.

sergeybykov · 2016-07-10T21:09:34Z

@normanhh3 I personally don't believe CLA is required for such a small change, but it would be easier if you could sign it. The process is very straightforward and painless. Thanks.

normanhh3 · 2016-07-11T15:39:02Z

Yeah, I am on it. Just needed to double check with employer first. :-/ Sigh.

Norman
On Jul 10, 2016 17:09, "Sergey Bykov" notifications@github.com wrote:

@normanhh3 https://github.com/normanhh3 I personally don't believe CLA
is required for such a small change, but it would be easier if you could
sign it. The process is very straightforward and painless. Thanks.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1923 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABj7xD1hCyWkoFZi8YjdSX_dXIcstW1Pks5qUV-ZgaJpZM4JIfUH
.

jdom · 2016-07-11T18:27:23Z

I don't believe structs are supported today in the code so adding this constraint to exclude them would not break existing functionality from my reading of the code.

Are you certain about this? I believe I've seen code that uses immutable structs as the state (they just set the entire State property on every modification).
I wouldn't want to break this feature just because, so unless we have a clear reason NOT to support structs, we shouldn't add that constraint

jdom · 2016-07-11T18:28:51Z

Also, if we are adding the new() constraint, maybe we should also use the new keyword to create new instances? or is this just too buried in the code to benefit from this generic constraint?

dnfclas · 2016-07-11T21:15:18Z

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

Thanks, DNFBOT;

normanhh3 · 2016-07-11T21:27:07Z

@jdom fair enough critique. Activator.CreateInstance(typeof(Int32)) does indeed return an Int.

The code here is where that state is created. I believe Line 79 is the relevant location.

Maybe the challenge here is JUST with the String type, although the error seemed to indicate that any class used for state had to have a parameterless constructor (by way of Activator.CreateInstance).

Maybe it should be struct, class, new() and Activator.CreateInstance can then appropriately determine how to construct struct's versus classes?

dVakulen · 2016-07-12T07:20:10Z

Just for some context - a while ago there used to be class, new() constrains, they were removed in this PR - #1060 (comment)

normanhh3 · 2016-07-12T11:37:31Z

The class constraint alone may have been the issue. I will play with all three and see if I can get it to support structs as well as classes.

normanhh3 · 2016-07-14T14:51:38Z

@dVakulen thanks for the clarification. @jdom, yes it appears that the intent is to support structs as well as classes. At the moment it does not appear that there will be a compatible compile time type check. :-( I was really hoping that struct, class, new() would do the trick and allow both structs and classes.

The only other thing I can think to do, to ease the debugging challenge is to clarify the exception message that is generated from Activator.CreateInstance to return something like "Hey, the Grain [GrainTypeHere] you attempted to initialize has a GrainState type [GrainStateTypeHere] that requires a parameterless constructor, please implement it."

Thoughts?

jdom · 2016-07-14T14:57:25Z

Not sure I understand the value of having both class and struct as constraints. Why not just new?

normanhh3 · 2016-07-14T20:21:37Z

@jdom this is likely stemming from my potential misunderstanding of the purpose of the new() constraint. My expectation is that structs would not meet that constraint. C# constructors for structs appears to be a new thing in C# 6.

I did compile the project with just the new() constraint and I did not get any solution compilation errors. I don't believe that is a 100% guaranteed way to ensure the constraint is appropriate.

Is there enough test coverage that I shouldn't need to add additional tests? I did review a number of them, but didn't see anything that was 100% what I was looking for as far as verification of supported Grain state types.

At this point, any pointers are appreciated.

jdom · 2016-07-14T21:12:41Z

well, I guess if the compiler enforces the constraint, it would be hard to create a unit test... If you can think of something to add coverage (now that you already browsed around the codebase) that would be welcome. But this additional constraint basically would disallow something that today was not covered (otherwise you would be getting compile time errors today)

jdom · 2016-07-14T21:13:38Z

Having said that, if there isn't any, it might be good to add a test using a struct for state.

normanhh3 · 2016-07-15T11:19:55Z

@jdom So far I have spent a few hours debugging what should be a simple test to indicate support of structs as state. I finally tracked down why my test is failing, but don't have a great explanation for why the code is the way it is.

Currently I have setup a test grain to use an Int64 to test the struct case with.

The following code from: SiloAssemblyLoader.cs Line 95

if (stateArg.GetTypeInfo().IsClass)
{
    grainStateType = stateArg;
    break;
}

Specifically the part where stateArg.GetTypeInfo().IsClass.

This line causes the struct-based grain state test to fail.

The issue here is that this code cannot determine the proper type for the grain because it does not support structs!

I propose changing the above code to be the following:

if (stateArg.GetTypeInfo().IsClass || stateArg.GetTypeInfo().IsValueType)
{
    grainStateType = stateArg;
    break;
}

This change allows my test to pass.

Unfortunately I am having an issue with bcyrpt.dll not being found properly so I can't run a full regression on the codebase right now.

[xUnit.net 00:00:00.0570519] TesterInternal: Catastrophic failure: System.DllNotFoundException: Unable to load DLL 'bcrypt.dll': The specified path is invalid. (Exception from HRESULT: 0x800700A1)
   at Microsoft.Win32.Win32Native.BCryptGetFipsAlgorithmMode(Boolean& pfEnabled)
   at System.Security.Cryptography.CryptoConfig.get_AllowOnlyFipsAlgorithms()
   at System.Security.Cryptography.CryptoConfig.get_DefaultNameHT()
   at System.Security.Cryptography.CryptoConfig.CreateFromName(String name, Object[] args)
   at System.Security.Policy.Hash.GenerateHash(Type hashType, Byte[] assemblyBytes)
   at System.Security.Policy.Hash.GenerateDefaultHashes(Byte[] assemblyBytes)
   at System.Security.Policy.Hash.GenerateDefaultHashes()
   at System.Security.Policy.Hash.OnSerializing(StreamingContext ctx)


   at System.AppDomain.get_Evidence()
   at System.AppDomain.get_EvidenceNoDemand()
   at System.AppDomain.get_Evidence()
   at Xunit.AppDomainManager_AppDomain.CreateAppDomain(String assemblyFilename, String configFilename, Boolean shadowCopy, String shadowCopyFolder)
   at Xunit.AppDomainManager_AppDomain..ctor(String assemblyFileName, String configFileName, Boolean shadowCopy, String shadowCopyFolder)
   at Xunit.DiaSessionWrapper..ctor(String assemblyFilename)
   at Xunit.VisualStudioSourceInformationProvider..ctor(String assemblyFileName)
   at Xunit.XunitFrontController..ctor(AppDomainSupport appDomainSupport, String assemblyFileName, String configFileName, Boolean shadowCopy, String shadowCopyFolder, ISourceInformationProvider sourceInformationProvider, IMessageSink diagnosticMessageSink)
   at Xunit.Runner.VisualStudio.TestAdapter.VsTestRunner.RunTestsInAssembly(IFrameworkHandle frameworkHandle, LoggerHelper logger, IMessageSink reporterMessageHandler, List`1 toDispose, AssemblyRunInfo runInfo)
========== Run test finished: 0 run (0:00:07.3113046) ==========

I will commit my changes and we can talk about how they might be good or bad.

…om structs

…ge capability with new() constraint

…ks implementing stateful grains. Fixes #1906

…om structs

…ge capability with new() constraint

jdom · 2016-07-15T17:15:57Z

test/TesterInternal/StorageTests/PersistenceGrainTests.cs

@@ -325,6 +325,26 @@ public void Persistence_Silo_StorageProvider_Name_Missing()
        }

        [Fact, TestCategory("Functional"), TestCategory("Persistence")]
+        public async Task Persistence_Read_Write_Struct_State()


For consistency, please use ValueType instead of Struct in the method name.

jdom · 2016-07-15T17:25:59Z

Thanks @normanhh3, adding coverage for value types is extremely useful. It seems (by the look at the dead code for IValueTypeTestGrain) that this was a feature that used to work, but probably when we made the GrainState base class obsolete, that we broke the feature without realizing, because there was no test using this grain.
This is speculation, I don't know exactly when we broke this feature. Nevertheless, I do think I remember someone using this feature maybe in February (granted, he might have been targeting and older version of Orleans).

BTW, not sure what that bcyrpt.dll issue is, never seen it before. Seems like the CI build didn't have the same issue anyway.

dVakulen · 2016-07-15T17:31:14Z

@jdom I believe that the root cause of broken support of value types is this line - a9128de#diff-2af53438f066a85e2bb283c98debde41R104

jdom · 2016-07-15T17:56:04Z

Wow, @dVakulen, that is a very old commit. Nevertheless I guess that wasn't an issue back then when you were required to inherit from GrainState (a class) and then compose with a value type.
Regardless of whether this used to work or not for the past year, I believe it's a noble goal to enable it anyway (assuming it's possible, which looks like @normanhh3 confirmed).
Maybe adding a few extra tests that explicitly read the state after a write, and also one that deactivates the grain and then re-activates and check that the value is preserved. Not saying @normanhh3 is on point for doing it, as he's already doing a lot more than he originally thought (thanks!). But if he's not, we probably should.

jdom · 2016-07-21T00:35:02Z

@dotnet-bot test this please
reason: removed dependency on MSTest since the PR was created

normanhh3 · 2016-07-21T01:52:31Z

Sorry guys, got caught up in some other major initiatives and haven't been able to get back to this yet.

ashkan-saeedi-mazdeh · 2016-08-14T09:10:26Z

@normanhh3 @jdom I saw an issue which I did not get the time to work more on it and fix or something. If grainState is an int then the serializer throws a NullReferenceException if you try to write state by WriteStateAsync before setting state to any value.

the grain was like

class MyGrain : Graind<int>,IMyGrainINterface
{
    pubcli Task Save()
    {
        return WriteStateAsync(); // throws NullReferenceException if called before setting State
    }
}

sergeybykov · 2016-08-20T00:53:01Z

@dotnet-bot test this please

…hh3/orleans into 1906-GrainStateConstraint

normanhh3 · 2016-08-21T13:54:24Z

@jdom check out the changes per your comments previously.

@sergeybykov it looks like I can't fix some test errors that are being created because the source for those tests is outside the repo! Your suggestions on how to resolve these remaining issues are appreciated.

    33>StorageTests\Relational\TestDataSets\GrainTypeGenerator.cs(21,23): error CS0310: 'TState' must be a non-abstract type with a public parameterless constructor in order to use it as parameter 'TGrainState' in the generic type or method 'Grain<TGrainState>' [D:\j\workspace\innerloop_prtest933994bb\test\TesterInternal\TesterInternal.csproj]
06:07:43     33>StorageTests\Relational\TestDataSets\GrainTypeGenerator.cs(22,23): error CS0310: 'TState' must be a non-abstract type with a public parameterless constructor in order to use it as parameter 'TGrainState' in the generic type or method 'Grain<TGrainState>' [D:\j\workspace\innerloop_prtest933994bb\test\TesterInternal\TesterInternal.csproj]
06:07:43     33>StorageTests\Relational\TestDataSets\GrainTypeGenerator.cs(28,23): error CS0310: 'TState' must be a non-abstract type with a public parameterless constructor in order to use it as parameter 'TGrainState' in the generic type or method 'Grain<TGrainState>' [D:\j\workspace\innerloop_prtest933994bb\test\TesterInternal\TesterInternal.csproj]
06:07:43     33>StorageTests\Relational\TestDataSets\GrainTypeGenerator.cs(29,23): error CS0310: 'TState' must be a non-abstract type with a public parameterless constructor in order to use it as parameter 'TGrainState' in the generic type or method 'Grain<TGrainState>' [D:\j\workspace\innerloop_prtest933994bb\test\TesterInternal\TesterInternal.csproj]
06:07:43     33>StorageTests\Relational\TestDataSets\GrainTypeGenerator.cs(35,23): error CS0310: 'TState' must be a non-abstract type with a public parameterless constructor in order to use it as parameter 'TGrainState' in the generic type or method 'Grain<TGrainState>' [D:\j\workspace\innerloop_prtest933994bb\test\TesterInternal\TesterInternal.csproj]
06:07:43     33>StorageTests\Relational\TestDataSets\GrainTypeGenerator.cs(36,23): error CS0310: 'TState' must be a non-abstract type with a public parameterless constructor in order to use it as parameter 'TGrainState' in the generic type or method 'Grain<TGrainState>' [D:\j\workspace\innerloop_prtest933994bb\test\TesterInternal\TesterInternal.csproj]
06:07:43     33>Done Building Project "D:\j\workspace\innerloop_prtest933994bb\test\TesterInternal\TesterInternal.csproj" (default targets) -- FAILED.
06:07:43      1>Done Building Project "D:\j\workspace\innerloop_prtest933994bb\src\Orleans.sln" (default targets) -- FAILED.

sergeybykov · 2016-09-30T15:34:32Z

@normanhh3 Looks like those types aren't even used anywhere. I deleted them, and everything build fine.

…ks implementing stateful grains. Fixes #1906

…ge capability with new() constraint

sergeybykov · 2016-09-30T16:27:18Z

@normanhh3 I rebased/squashed your commits in fixed the build break in https://github.com/sergeybykov/orleans/commits/1923-rebased. Feel free to pull into your branch.

normanhh3 · 2016-09-30T20:51:24Z

Awesome, I'll pull it into my code once I figure out how. ;-)

sergeybykov · 2016-09-30T21:15:05Z

If it's too much hassle for you, I can simply submit a PR with my branch in lieu of this one.

sergeybykov · 2016-10-01T05:20:09Z

Thank you, @normanhh3!

…ints - Resolves dotnet#1906 (dotnet#1923) * Add TGrainState constraints to document clearly what is needed by folks implementing stateful grains. Fixes dotnet#1906 * Removed class constraint because it was overly broad * Extended type interogation to determine the appropriate grain type from structs * Created test case to verify support of struct-based grain state storage capability with new() constraint * Add TGrainState constraints to document clearly what is needed by folks implementing stateful grains. Fixes dotnet#1906 * Removed class constraint because it was overly broad * Extended type interogation to determine the appropriate grain type from structs * Created test case to verify support of struct-based grain state storage capability with new() constraint * Moved test grain to non-internal location * Switched to using ValueTypeTestData * Moved test to new location and refactored to use non-internal methods * Add TGrainState constraints to document clearly what is needed by folks implementing stateful grains. Fixes dotnet#1906 * Created test case to verify support of struct-based grain state storage capability with new() constraint * Removed unused grain classes that were failing to compile. * Fixed MemoryStore provider name.

Add TGrainState constraints to document clearly what is needed by fol…

d4e20fe

…ks implementing stateful grains. Fixes #1906

dnfclas added the cla-required label Jul 9, 2016

jthelin added the enhancement label Jul 10, 2016

dnfclas added cla-signed and removed cla-required labels Jul 11, 2016

normanhh3 mentioned this pull request Jul 14, 2016

GrainState generic type constraint #1906

Closed

normanhh3 added 7 commits July 15, 2016 07:25

Removed class constraint because it was overly broad

874c9f3

Extended type interogation to determine the appropriate grain type fr…

69e272f

…om structs

Created test case to verify support of struct-based grain state stora…

b52fa3d

…ge capability with new() constraint

Add TGrainState constraints to document clearly what is needed by fol…

86501fe

…ks implementing stateful grains. Fixes #1906

Removed class constraint because it was overly broad

117e405

Extended type interogation to determine the appropriate grain type fr…

ba21211

…om structs

Created test case to verify support of struct-based grain state stora…

0e865d5

…ge capability with new() constraint

jdom reviewed Jul 15, 2016
View reviewed changes

sergeybykov assigned jdom Jul 21, 2016

normanhh3 added 4 commits August 21, 2016 08:02

Merge branch '1906-GrainStateConstraint' of https://github.com/norman…

a0f1c25

…hh3/orleans into 1906-GrainStateConstraint

Moved test grain to non-internal location

36974bc

Switched to using ValueTypeTestData

39b3d73

Moved test to new location and refactored to use non-internal methods

8f878de

normanhh3 and others added 3 commits September 30, 2016 08:56

Add TGrainState constraints to document clearly what is needed by fol…

961ecdd

…ks implementing stateful grains. Fixes #1906

Created test case to verify support of struct-based grain state stora…

34a9f3f

…ge capability with new() constraint

Removed unused grain classes that were failing to compile.

fd29e2e

sergeybykov and others added 2 commits September 30, 2016 14:31

Fixed MemoryStore provider name.

19d672b

Pulled from rebase by sergeybykov to correct issues

538167f

sergeybykov merged commit 4b991d4 into dotnet:master Oct 1, 2016

normanhh3 deleted the 1906-GrainStateConstraint branch October 3, 2016 10:00

github-actions bot locked and limited conversation to collaborators Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TGrainState type constraints to document necessary system constraints - Resolves #1906 #1923

Add TGrainState type constraints to document necessary system constraints - Resolves #1906 #1923

normanhh3 commented Jul 9, 2016

dnfclas commented Jul 9, 2016

gabikliot commented Jul 10, 2016

normanhh3 commented Jul 10, 2016

sergeybykov commented Jul 10, 2016

normanhh3 commented Jul 11, 2016

jdom commented Jul 11, 2016

jdom commented Jul 11, 2016

dnfclas commented Jul 11, 2016

normanhh3 commented Jul 11, 2016 •

edited

Loading

dVakulen commented Jul 12, 2016 •

edited

Loading

normanhh3 commented Jul 12, 2016

normanhh3 commented Jul 14, 2016

jdom commented Jul 14, 2016

normanhh3 commented Jul 14, 2016

jdom commented Jul 14, 2016

jdom commented Jul 14, 2016

normanhh3 commented Jul 15, 2016

jdom Jul 15, 2016

jdom commented Jul 15, 2016

dVakulen commented Jul 15, 2016 •

edited

Loading

jdom commented Jul 15, 2016

jdom commented Jul 21, 2016

normanhh3 commented Jul 21, 2016

ashkan-saeedi-mazdeh commented Aug 14, 2016 •

edited

Loading

sergeybykov commented Aug 20, 2016

normanhh3 commented Aug 21, 2016

sergeybykov commented Sep 30, 2016

sergeybykov commented Sep 30, 2016

normanhh3 commented Sep 30, 2016

sergeybykov commented Sep 30, 2016

sergeybykov commented Oct 1, 2016

Add TGrainState type constraints to document necessary system constraints - Resolves #1906 #1923

Add TGrainState type constraints to document necessary system constraints - Resolves #1906 #1923

Conversation

normanhh3 commented Jul 9, 2016

dnfclas commented Jul 9, 2016

gabikliot commented Jul 10, 2016

normanhh3 commented Jul 10, 2016

sergeybykov commented Jul 10, 2016

normanhh3 commented Jul 11, 2016

jdom commented Jul 11, 2016

jdom commented Jul 11, 2016

dnfclas commented Jul 11, 2016

normanhh3 commented Jul 11, 2016 • edited Loading

dVakulen commented Jul 12, 2016 • edited Loading

normanhh3 commented Jul 12, 2016

normanhh3 commented Jul 14, 2016

jdom commented Jul 14, 2016

normanhh3 commented Jul 14, 2016

jdom commented Jul 14, 2016

jdom commented Jul 14, 2016

normanhh3 commented Jul 15, 2016

jdom Jul 15, 2016

Choose a reason for hiding this comment

jdom commented Jul 15, 2016

dVakulen commented Jul 15, 2016 • edited Loading

jdom commented Jul 15, 2016

jdom commented Jul 21, 2016

normanhh3 commented Jul 21, 2016

ashkan-saeedi-mazdeh commented Aug 14, 2016 • edited Loading

sergeybykov commented Aug 20, 2016

normanhh3 commented Aug 21, 2016

sergeybykov commented Sep 30, 2016

sergeybykov commented Sep 30, 2016

normanhh3 commented Sep 30, 2016

sergeybykov commented Sep 30, 2016

sergeybykov commented Oct 1, 2016

normanhh3 commented Jul 11, 2016 •

edited

Loading

dVakulen commented Jul 12, 2016 •

edited

Loading

dVakulen commented Jul 15, 2016 •

edited

Loading

ashkan-saeedi-mazdeh commented Aug 14, 2016 •

edited

Loading