Akka.Serialization: `INoSerializationVerificationNeeded` does not handle `IWrappedMessage` correctly #7010

Aaronontheweb · 2023-11-29T22:27:55Z

Changes

INoSerializationVerificationNeeded does not handle IWrappedMessage correctly. We handle DeadLetter correctly as a special case, but not any of the other IWrappedMessage implementations. We should handle this generically.

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.

`INoSerializationVerificationNeeded` does not handle `IWrappedMessage` correctly

Aaronontheweb · 2023-11-29T22:28:09Z

Added a repro for this; working on a fix now.

Aaronontheweb

Detailed my changes.

Aaronontheweb · 2023-11-29T22:33:09Z

src/core/Akka.Tests/Serialization/SerializeAllMessagesSpec.cs

+        }
+
+        // add an unserializable member, such as a delegate
+        public Action MyAction { get; }


Newtonsoft.Json and Hyperion will both barf on delegate serialization, so this message will not be received when the parent IWrappedMessage is not handled correctly.

Aaronontheweb · 2023-11-29T22:33:24Z

src/core/Akka.Tests/Serialization/SerializeAllMessagesSpec.cs

+
+public class SerializeAllMessagesSpec : AkkaSpec
+{
+    public SerializeAllMessagesSpec(ITestOutputHelper output) : base("akka.actor.serialize-messages = on", output)


Have to enable akka.actor.serialize-messages=on in order for this to even be an issue.

Aaronontheweb · 2023-11-29T22:33:54Z

src/core/Akka.Tests/Serialization/SerializeAllMessagesSpec.cs

+    {
+        // Arrange
+        var myProbe = CreateTestProbe();
+        var action = () => { myProbe.Tell("worked"); };


Invoking the delegate closes on the loop on this spec by messaging the closed-over TestProbe.

Aaronontheweb · 2023-11-29T22:34:39Z

src/core/Akka.Tests/Serialization/SerializeAllMessagesSpec.cs

+        {
+            // Act
+            myActor.Tell(wrappedMessage);
+            await myProbe.ExpectMsgAsync("worked");


Both the EventFilter will fail and the ExpectMsgAsync will fail if the ActorCell of the recipient attempts to serialize this message - which is exactly what happened before I applied my fix.

Aaronontheweb · 2023-11-29T22:35:12Z

src/core/Akka/Actor/ActorCell.cs

-            var unwrapped = (deadLetter = envelope.Message as DeadLetter) != null ? deadLetter.Message : envelope.Message;
+            var unwrapped = envelope.Message;
+
+            if(envelope.Message is IWrappedMessage wrapped)


for all IWrappedMessages: if the underlying message doesn't want to be serialized (typically because it can't be), don't try it. Just return the message without performing any additional work.

Aaronontheweb · 2023-11-29T22:35:29Z

src/core/Akka/Actor/ActorCell.cs

@@ -534,7 +537,8 @@ private Envelope SerializeAndDeserialize(Envelope envelope)
                throw new SerializationException($"Failed to serialize and deserialize payload object [{unwrapped.GetType()}]. Envelope: [{envelope}], Actor type: [{Actor.GetType()}]", e);
            }

-            if (deadLetter != null)
+            // special case handling for DeadLetters
+            if (envelope.Message is DeadLetter deadLetter)


Restore the special-case handling for DeadLetters that we had before.

…ionNeeded

Arkatufus · 2023-11-30T16:29:54Z

src/core/Akka/Actor/ActorCell.cs

@@ -534,7 +537,8 @@ private Envelope SerializeAndDeserialize(Envelope envelope)
                throw new SerializationException($"Failed to serialize and deserialize payload object [{unwrapped.GetType()}]. Envelope: [{envelope}], Actor type: [{Actor.GetType()}]", e);
            }

-            if (deadLetter != null)
+            // special case handling for DeadLetters
+            if (envelope.Message is DeadLetter deadLetter)


It seems like we're missing a round of unwrap here, this code will return an Envelope that wraps a DeadLetter that wraps a DeadLetter.
The DeadLetter message still needs to be deserialized like it was before and then re-wrapped inside a new DeadLetter message.

Good point, I can use the Unwrapped utility from earlier to recursively unwrap it - let me look into that. Currently doing a full test suite run locally so I can fix some of the outstanding .NET 8 errors in the suite.

…ionNeeded

Arkatufus

Self review

Arkatufus · 2023-12-05T16:19:02Z

src/core/Akka/Actor/BuiltInActors.cs

+            while (message is IWrappedMessage wm)
+            {
+                message = wm.Message;
+            }


Unwrap tail recursion into loop

Arkatufus · 2023-12-05T16:19:49Z

src/core/Akka/Actor/ActorCell.cs

+            // special case handling for DeadLetters
+            return deadLetter is not null 
+                ? new Envelope(new DeadLetter(deserializedMsg, deadLetter.Sender, deadLetter.Recipient), envelope.Sender) 
+                : new Envelope(deserializedMsg, envelope.Sender);


This code behaves exactly like the old code, only modernized

Looks fine to me

Arkatufus · 2023-12-05T16:20:35Z

src/core/Akka/Actor/ActorCell.cs

+            var deadLetter = envelope.Message as DeadLetter;
+
+            // recursively unwraps message
+            var unwrapped = WrappedMessage.Unwrap(deadLetter is not null ? deadLetter.Message : envelope.Message);


If the envelope message is a DeadLetter, grab the dead letter message instead.

Also unwraps the message recursively if it is an IWrappedMessage. There is no need to check if the message is IWrappedMessage or not, it is done inside the Unwrap static function.

Don't need to check for a DeadLetter here as it implements IWrappedMessage.

Arkatufus · 2023-12-05T16:21:31Z

src/core/Akka/Actor/ActorCell.cs

-            DeadLetter deadLetter;
-            var unwrapped = (deadLetter = envelope.Message as DeadLetter) != null ? deadLetter.Message : envelope.Message;
-
+            var deadLetter = envelope.Message as DeadLetter;


Check to see if the envelope message is a DeadLetter

Aaronontheweb

Just one nitpick on the duplicate check for a DeadLetter @Arkatufus

Aaronontheweb · 2023-12-06T02:17:36Z

src/core/Akka/Actor/ActorCell.cs

+            var deadLetter = envelope.Message as DeadLetter;
+
+            // recursively unwraps message
+            var unwrapped = WrappedMessage.Unwrap(deadLetter is not null ? deadLetter.Message : envelope.Message);


Don't need to check for a DeadLetter here as it implements IWrappedMessage.

Aaronontheweb · 2023-12-06T02:17:59Z

src/core/Akka/Actor/ActorCell.cs

+            // special case handling for DeadLetters
+            return deadLetter is not null 
+                ? new Envelope(new DeadLetter(deserializedMsg, deadLetter.Sender, deadLetter.Recipient), envelope.Sender) 
+                : new Envelope(deserializedMsg, envelope.Sender);


Looks fine to me

…ionNeeded

need to serialize both the envelope and the underlying type when `serialize-messages=on`

…ng type when using `serialize-messages=on` (#7200) * Fix bug introduced by #7010 need to serialize both the envelope and the underlying type when `serialize-messages=on` * handle edge case with `IWrappedMessage` that implements `INoSerializationVerificationNeeded` * Add unit test * Fix pub-sub router spec --------- Co-authored-by: Gregorius Soedharmo <arkatufus@yahoo.com>

Added reproduction

b36a0c8

`INoSerializationVerificationNeeded` does not handle `IWrappedMessage` correctly

Aaronontheweb added confirmed bug serialization labels Nov 29, 2023

Aaronontheweb added this to the 1.5.14 milestone Nov 29, 2023

fixed issue

459007b

Aaronontheweb modified the milestones: 1.5.14, 1.5.15 Nov 29, 2023

Aaronontheweb commented Nov 29, 2023

View reviewed changes

Aaronontheweb added 2 commits November 29, 2023 16:47

added correct unwrapping method

c8462ce

Merge branch 'dev' into fix-IWrappedMessage-INoSerializationVerificat…

e3fc42f

…ionNeeded

Arkatufus requested changes Nov 30, 2023

View reviewed changes

Aaronontheweb and others added 2 commits November 30, 2023 14:19

Merge branch 'dev' into fix-IWrappedMessage-INoSerializationVerificat…

d12a13f

…ionNeeded

Fix dead letter behaviour change

2b331a6

Arkatufus approved these changes Dec 5, 2023

View reviewed changes

Arkatufus reviewed Dec 5, 2023

View reviewed changes

Aaronontheweb commented Dec 6, 2023

View reviewed changes

Arkatufus added 2 commits December 7, 2023 01:58

Fix implementation

e9ec8ff

Merge branch 'dev' into fix-IWrappedMessage-INoSerializationVerificat…

70a4e69

…ionNeeded

Aaronontheweb merged commit 04630da into akkadotnet:dev Dec 7, 2023

Aaronontheweb deleted the fix-IWrappedMessage-INoSerializationVerificationNeeded branch December 7, 2023 13:01

Aaronontheweb mentioned this pull request Jan 10, 2024

added v1.5.15 release notes #7053

Merged

Aaronontheweb mentioned this pull request May 23, 2024

Akka.Actor.Serialization: remember to serialize envelope AND underlying type when using serialize-messages=on #7200

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Akka.Serialization: `INoSerializationVerificationNeeded` does not handle `IWrappedMessage` correctly #7010

Akka.Serialization: `INoSerializationVerificationNeeded` does not handle `IWrappedMessage` correctly #7010

Aaronontheweb commented Nov 29, 2023 •

edited

Loading

Aaronontheweb commented Nov 29, 2023

Aaronontheweb left a comment

Aaronontheweb Nov 29, 2023

Aaronontheweb Nov 29, 2023

Aaronontheweb Nov 29, 2023

Aaronontheweb Nov 29, 2023

Aaronontheweb Nov 29, 2023

Aaronontheweb Nov 29, 2023

Arkatufus Nov 30, 2023

Aaronontheweb Nov 30, 2023

Arkatufus left a comment

Arkatufus Dec 5, 2023

Aaronontheweb Dec 6, 2023

Arkatufus Dec 5, 2023

Aaronontheweb Dec 6, 2023

Arkatufus Dec 5, 2023

Arkatufus Dec 5, 2023

Aaronontheweb Dec 6, 2023

Arkatufus Dec 5, 2023

Aaronontheweb left a comment

Aaronontheweb Dec 6, 2023

Aaronontheweb Dec 6, 2023

Akka.Serialization: INoSerializationVerificationNeeded does not handle IWrappedMessage correctly #7010

Akka.Serialization: INoSerializationVerificationNeeded does not handle IWrappedMessage correctly #7010

Conversation

Aaronontheweb commented Nov 29, 2023 • edited Loading

Changes

Checklist

Aaronontheweb commented Nov 29, 2023

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

Choose a reason for hiding this comment

Arkatufus 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

Choose a reason for hiding this comment

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

Akka.Serialization: `INoSerializationVerificationNeeded` does not handle `IWrappedMessage` correctly #7010

Akka.Serialization: `INoSerializationVerificationNeeded` does not handle `IWrappedMessage` correctly #7010

Aaronontheweb commented Nov 29, 2023 •

edited

Loading