`Exception` serialization support for built-in messages #6297

Aaronontheweb · 2022-12-08T02:19:26Z

Changes

Adding support to any built-in messages that support Exception types. Won't be adding any serialization magic to Newtonsoft.Json for exception handling as that starts to get into CVE territory given the deep amount of context the serialization payloads contain.

Some of the designs we're considering for 1.5 / later, namely using source generators to create compile-time serialization for everything, will make this issue easier to manage in the future. For now I think it's good to preserve the location transparency of Akka.NET's own built-in messages for end-user convenience and consistency.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

This change follows the Akka.NET API Compatibility Guidelines.
This change follows the Akka.NET Wire Compatibility Guidelines.
I have reviewed my own pull request.
Design discussion issue Remoting and an exception as a payload message #3903

…r supervision

Aaronontheweb · 2022-12-08T02:19:51Z

src/core/Akka.Remote.Tests/Serialization/Bugfix3903Spec.cs

+
+        // a test where Sys starts a ParentActor and has it remotely deploy an EchoActor onto a second ActorSystem
+        [Fact]
+        public async Task ParentActor_should_be_able_to_deploy_EchoActor_to_remote_system()


End to end test that validates that system messages are currently covered already.

Aaronontheweb · 2022-12-08T02:21:34Z

Going to define some new Protos for the following message types:

Status.Failure
Status.Success

If there are any other built-in messages that should be covered I'd be happy to handle those here - please let me know!

…ageSerializer`

Aaronontheweb

Detailed my changes

Aaronontheweb · 2022-12-08T03:00:25Z

src/core/Akka.Remote.Tests/Serialization/MiscMessageSerializerSpec.cs

+        [InlineData(null)]
+        [InlineData(1)]
+        [InlineData("hi")]
+        public void Can_serialize_StatusSuccess(object payload)


Try with a variety of payloads for Status.Success

Aaronontheweb · 2022-12-08T03:00:35Z

src/core/Akka.Remote.Tests/Serialization/MiscMessageSerializerSpec.cs

+        [InlineData(null)]
+        [InlineData(1)]
+        [InlineData("hi")]
+        public void Can_serialize_StatusFailure(object payload)


Try with a variety of payloads for Status.Failure

Aaronontheweb · 2022-12-08T03:01:06Z

src/protobuf/ContainerFormats.proto

@@ -70,3 +70,12 @@ message ExceptionData {
  ExceptionData innerException = 5;
  map<string, Payload> customFields = 6;
 }
+
+message StatusSuccess{


New .proto message formats for Status.Success and Status.Failure

Aaronontheweb · 2022-12-08T03:05:17Z

src/core/Akka.Remote/Configuration/Remote.conf

@@ -39,6 +39,8 @@ akka {
      "Akka.Actor.PoisonPill, Akka" = akka-misc
      "Akka.Actor.Kill, Akka" = akka-misc
      "Akka.Actor.PoisonPill, Akka" = akka-misc
+      "Akka.Actor.Status+Failure, Akka" = akka-misc


New serialization bindings for these types.

So in terms of wire implications:

Any old Status.Failure / Status.Success messages sent over the network using Newtonsoft.Json will still be deserialized as Newtonsoft.Json when they arrive on an upgraded server - no issues there. Forwards-compatible.

Any new Status.Failure / Status.Success messages sent using the Protobuf format to a server running Akka.NET v1.4.46 or earlier will see these messages dropped until they upgrade.

I can't really use a feature flag here since this is changing the bindings themselves - so users might hit that problem when they upgrade. Given that Status.Failure already mostly can't be serialized today I think this is likely a non-issue for most users. My vote is to just push this change out there.

Can't we still use feature flags if we programmatically inject these bindings using Serialization.AddSerializationMap()?

I sincerely don't think the juice is worth the squeeze to do it, especially given that 1.5 is just around the corner.

Aaronontheweb · 2022-12-08T03:05:44Z

cc @Zetanova - what do you think of this?

Arkatufus

Looks good overall, got a question.

Arkatufus · 2022-12-08T20:20:55Z

src/core/Akka.Remote/Configuration/Remote.conf

@@ -39,6 +39,8 @@ akka {
      "Akka.Actor.PoisonPill, Akka" = akka-misc
      "Akka.Actor.Kill, Akka" = akka-misc
      "Akka.Actor.PoisonPill, Akka" = akka-misc
+      "Akka.Actor.Status+Failure, Akka" = akka-misc


Can't we still use feature flags if we programmatically inject these bindings using Serialization.AddSerializationMap()?

Zetanova · 2022-12-08T23:01:41Z

@Aaronontheweb The issue itself is not addressed.
If the remote actor throws a unknown exception type or inner-exception
back to the caller. The caller will not receive it.

The remote actor itself does not know or better to say should not to know
because everything is "transparent"

The current effect is that the remote connection will fail with a deserializing exception
and the caller will never see a response message.
In a cluster this would lead to a disassociation and a required restart of the node.

Zetanova · 2022-12-08T23:16:02Z

@Aaronontheweb

akka.net/src/core/Akka.Remote/Serialization/ExceptionSupport.cs

Lines 109 to 127 in fbc338a

    
           public Exception ExceptionFromProtoNet(Proto.Msg.ExceptionData proto) 
        
           { 
        
               if (string.IsNullOrEmpty(proto.TypeName)) 
        
                   return null; 
        
               Type exceptionType = Type.GetType(proto.TypeName); 
        
               var serializationInfo = new SerializationInfo(exceptionType, _defaultFormatterConverter); 
        
               serializationInfo.AddValue("ClassName", proto.TypeName); 
        
               serializationInfo.AddValue("Message", ValueOrNull(proto.Message)); 
        
               serializationInfo.AddValue("StackTraceString", ValueOrNull(proto.StackTrace)); 
        
               serializationInfo.AddValue("Source", ValueOrNull(proto.Source)); 
        
               serializationInfo.AddValue("InnerException", ExceptionFromProto(proto.InnerException)); 
        
               serializationInfo.AddValue("HelpURL", string.Empty); 
        
               serializationInfo.AddValue("RemoteStackTraceString", null); 
        
               serializationInfo.AddValue("RemoteStackIndex", 0); 
        
               serializationInfo.AddValue("ExceptionMethod", null); 
        
               serializationInfo.AddValue("HResult", int.MinValue);

This method should only TryToDeserialize if the TypeName can be resolved.
If not a AkkaGenericException should be created

My Idea was to replace the Exception instance to a basic Reason instance on the producing actor.
It can be tagged by an Id and stored into a memory cache or something else defined by the users config (like elasticsearch)

For the receiver it would the be OPTIONAL to convert Reason back to an Exception instance
and this could always be over an TryConvertReasonToException pattern.

Aaronontheweb · 2022-12-08T23:53:27Z

@Aaronontheweb The issue itself is not addressed. If the remote actor throws a unknown exception type or inner-exception back to the caller. The caller will not receive it.

The remote actor itself does not know or better to say should not to know because everything is "transparent"

We're not going to recursively nest exceptions all the way down the stack - sorry, but we have to draw the line somewhere. We don't want to be in the already messy business + dangerous of serializing exceptions.

The current effect is that the remote connection will fail with a deserializing exception and the caller will never see a response message. In a cluster this would lead to a disassociation and a required restart of the node.

This is not true - Akka.Remote does not drop associations when deserialization exceptions occur. That's been fixed a ways back.

Zetanova · 2022-12-09T01:55:38Z

We're not going to recursively nest exceptions all the way down the stack - sorry, but we have to draw the line somewhere. We don't want to be in the already messy business + dangerous of serializing exceptions.

My proposal to convert the exceptions to basic Reason messages is exactly the opposite.
This would already happen in the PostStop of the ActorBase and then escalated.

The Serializer/Wire would in the default config then never see a single exception.
The conversion back to an exception would be then optional and more or less for legacy use
and supported only with an extensions.

Instead of a simple Reason type an AkkaGenericException could be made.
It should only be a WellKnown type for all nodes and could hold an Error-Guid, Error-Code and Error-Message
and never a single InnerException

Aaronontheweb · 2022-12-09T14:08:59Z

The Serializer/Wire would in the default config then never see a single exception.
The conversion back to an exception would be then optional and more or less for legacy use
and supported only with an extensions.

So we don't have a formal spec for this yet as we think it's out of scope for Akka.NET v1.5, but we're looking at replacing the "DIY" serialization system with something more authoritative (source generators) in the future. We'd have a formula for working with Exception types as part of that most likely.

) * added end to end spec to validate error serialization for remote actor supervision * defined `.proto`s for `Status.Failure` and `Status.Success` * added `Status.,Failure` and `Status.Success` support to the `MiscMessageSerializer` * added tests to `MiscMessageSerializerSpec` * close akkadotnet#3903 (cherry picked from commit 998dcca)

* added end to end spec to validate error serialization for remote actor supervision * defined `.proto`s for `Status.Failure` and `Status.Success` * added `Status.,Failure` and `Status.Success` support to the `MiscMessageSerializer` * added tests to `MiscMessageSerializerSpec` * close #3903 (cherry picked from commit 998dcca) Co-authored-by: Aaron Stannard <aaron@petabridge.com>

added end to end spec to validate error serialization for remote acto…

2d0906e

…r supervision

Aaronontheweb added akka-remote serialization akka.net v1.4 Issues affecting Akka.NET v1.4 labels Dec 8, 2022

Aaronontheweb commented Dec 8, 2022

View reviewed changes

Aaronontheweb added 4 commits December 7, 2022 20:24

defined .protos for Status.Failure and Status.Success

099943a

added Status.,Failure and Status.Success support to the `MiscMess…

91e4756

…ageSerializer`

added tests to MiscMessageSerializerSpec

f34970d

close akkadotnet#3903

fbc338a

Aaronontheweb marked this pull request as ready for review December 8, 2022 02:59

Aaronontheweb commented Dec 8, 2022

View reviewed changes

Arkatufus requested changes Dec 8, 2022

View reviewed changes

Arkatufus approved these changes Dec 8, 2022

View reviewed changes

Aaronontheweb merged commit 998dcca into akkadotnet:v1.4 Dec 8, 2022

Aaronontheweb deleted the remote-error-serialization branch December 8, 2022 23:53

Arkatufus mentioned this pull request Dec 9, 2022

[PORT #6297] Exception serialization support for built-in messages #6300

Merged

Aaronontheweb mentioned this pull request May 16, 2023

"System.Runtime.Serialization.SerializationException: Failed to deserialize object of type [Akka.Actor.Status+Failure] from the stream. Cause: Index was outside the bounds of the array." After Akka Upgrade from 1.3.14 to 1.4.48 #6760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Exception` serialization support for built-in messages #6297

`Exception` serialization support for built-in messages #6297

Aaronontheweb commented Dec 8, 2022 •

edited

Loading

Aaronontheweb Dec 8, 2022

Aaronontheweb commented Dec 8, 2022 •

edited

Loading

Aaronontheweb left a comment

Aaronontheweb Dec 8, 2022

Aaronontheweb Dec 8, 2022

Aaronontheweb Dec 8, 2022

Aaronontheweb Dec 8, 2022

Arkatufus Dec 8, 2022

Aaronontheweb Dec 8, 2022

Arkatufus Dec 8, 2022

Aaronontheweb commented Dec 8, 2022

Arkatufus left a comment

Arkatufus Dec 8, 2022

Zetanova commented Dec 8, 2022

Zetanova commented Dec 8, 2022

Aaronontheweb commented Dec 8, 2022

Zetanova commented Dec 9, 2022

Aaronontheweb commented Dec 9, 2022

Exception serialization support for built-in messages #6297

Exception serialization support for built-in messages #6297

Conversation

Aaronontheweb commented Dec 8, 2022 • edited Loading

Changes

Checklist

Choose a reason for hiding this comment

Aaronontheweb commented Dec 8, 2022 • edited Loading

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb commented Dec 8, 2022

Arkatufus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zetanova commented Dec 8, 2022

Zetanova commented Dec 8, 2022

Aaronontheweb commented Dec 8, 2022

Zetanova commented Dec 9, 2022

Aaronontheweb commented Dec 9, 2022

`Exception` serialization support for built-in messages #6297

`Exception` serialization support for built-in messages #6297

Aaronontheweb commented Dec 8, 2022 •

edited

Loading

Aaronontheweb commented Dec 8, 2022 •

edited

Loading