dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.06k stars 2.03k forks source link

[4.0] Warning or errors for missing serializers #7584

Closed SebastianStehle closed 2 years ago

SebastianStehle commented 2 years ago

Hi,

I started to migrate the Orleans Dashboard to get used with changes in Orleans 4.0 and one thing is confusing.

In 3.X everything just worked automatically. This was a good and bad thing. But it was consistent. Now you have to annotate types, but there are no checks or warnings if I have forgotten an annotation. But for local development it also does not matter, because usually you have only one node and there is no serialization involved. But I guess it will crash in production or if you are lucky and you have an api test with more than one node it will crash there.

Do I miss something?

ReubenBond commented 2 years ago

You didn't miss anything, there's no attempt by the system to warn you that a type will not be able to be serialized. There are a couple of reasons for that:

If you can come up with a solution, I'd love to hear it. The serializer does provide a hint for types marked [Serializable], so it's possible we could upgrade that to a warning via an project switch.

SebastianStehle commented 2 years ago

I would say a false warning would be better than no warnings and with roslyn you can also disable them. So I would just output a warning for every type that is used in a Grain interface and has neither a [GenerateSerializer] attribute nor is a supported framework type of the serializer.

ReubenBond commented 2 years ago

That could work. We could also configure the behavior via a csproj flag. I'll see about it. Did you close this on purpose? I think we should improve the experience here.

SebastianStehle commented 2 years ago

No, I just pressed the wrong button by accident.

SebastianStehle commented 2 years ago

I have forgotten 2 types in the Orleans Dashboard and have realized it after implementing a sample with a dedicated Client. Just wanna point out that this is important ;)

ReubenBond commented 2 years ago

The biggest blocker for this right now is this issue: https://github.com/dotnet/roslyn/issues/49664 Without being able to emit those diagnostics directly from the Source Generator, we'd require a complex 2-pass process in the Orleans.Analyzers project.

Another option would be a warning on startup for types without serializers, which would be more accurate since it knows the runtime configuration, is rather straightforward to implement, but may slow down startup and comes late in the process and is less convenient.

EDIT: please upvote the Roslyn issue if this is important to you

SebastianStehle commented 2 years ago

ASP.NET Core does something similar or to be more precise, the default ServiceProvider. In Development mode, they check whether all services can be build:

So I would do something similar.

ReubenBond commented 2 years ago

It's certainly not as developer-friendly, since an error message at runtime can't take you to the right place in code (where the class is defined), but I'm fine with adding it since at least it will throw.

SebastianStehle commented 2 years ago

Perfect is the enemy of good ;)

ReubenBond commented 2 years ago

Right, the choice is not between good and perfect. Neither option is perfect. It's a question of developer experience and prioritization. Ultimately, we can have both, at some expense and with the flaws of each option and granted that the Roslyn issue is rectified.

ReubenBond commented 2 years ago

Here's an example of what this may look like:

    Orleans.Runtime.OrleansConfigurationException : Found unserializable or uncopyable types which are being referenced in grain interface signatures:
Type: Orleans.UnitTest.GrainInterfaces.MyTypeWithAPrivateTypeField has no serializer or copier and was referenced by the following methods:
    * Orleans.UnitTest.GrainInterfaces.IPrivateReturnType,TestGrainInterfaces.Foo
Type: UnitTests.GrainInterfaces.One.Command has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISameNameParameterTypeGrain,TestGrainInterfaces.ExecuteCommand
Type: UnitTests.GrainInterfaces.Two.Command has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISameNameParameterTypeGrain,TestGrainInterfaces.ExecuteCommand
Type: Outsider has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISomeGrain,TestGrainInterfaces.Do
Type: UnitTests.GrainInterfaces.SomeTypeUsedInGrainInterface has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.IConsiderForCodeGenerationGrain,TestGrainInterfaces.SomeGrainCall
Type: Newtonsoft.Json.Linq.JObject has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.IJsonEchoGrain,TestGrainInterfaces.EchoJson
Type: UnitTests.GrainInterfaces.AB has no serializer and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ILogTestGrain,TestGrainInterfaces.GetBothGlobal
    * UnitTests.GrainInterfaces.ILogTestGrain,TestGrainInterfaces.GetBothLocal
Type: UnitTests.GrainInterfaces.SerializerTestClass1 has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISerializationGeneratorTaskTest,TestGrainInterfaces.Method1
Type: UnitTests.GrainInterfaces.SerializerTestClass2 has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISerializationGeneratorTaskTest,TestGrainInterfaces.Method1
Type: UnitTests.GrainInterfaces.SerializerTestClass3 has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISerializationGeneratorTaskTest,TestGrainInterfaces.Method2
Type: UnitTests.GrainInterfaces.SerializerTestClass4 has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISerializationGeneratorPromiseTest,TestGrainInterfaces.Method1
Type: UnitTests.GrainInterfaces.SerializerTestClass5 has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISerializationGeneratorPromiseTest,TestGrainInterfaces.Method1
Type: UnitTests.GrainInterfaces.SerializerTestClass6 has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISerializationGeneratorPromiseTest,TestGrainInterfaces.Method2
Type: UnitTests.GrainInterfaces.MyType has no serializer or copier and was referenced by the following methods:
    * UnitTests.GrainInterfaces.ISimpleTestTempGrain,TestInternalGrainInterfaces.SimpleMethod
SebastianStehle commented 2 years ago

Awesome, thank you :)