Skip to content
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 Akka.Remote serialization exception bubbling and messages #5072

Merged
merged 10 commits into from Jun 9, 2021
1 change: 1 addition & 0 deletions src/core/Akka.Remote.Tests/Akka.Remote.Tests.csproj
Expand Up @@ -8,6 +8,7 @@
</PropertyGroup>

<ItemGroup>
<ProjectReference Include="..\..\contrib\cluster\Akka.DistributedData\Akka.DistributedData.csproj" />
Arkatufus marked this conversation as resolved.
Show resolved Hide resolved
<ProjectReference Include="..\Akka.Remote\Akka.Remote.csproj" />
<ProjectReference Include="..\Akka.Tests.Shared.Internals\Akka.Tests.Shared.Internals.csproj" />

Expand Down
97 changes: 97 additions & 0 deletions src/core/Akka.Remote.Tests/Serialization/BugFix5062Spec.cs
@@ -0,0 +1,97 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Runtime.Serialization;
using System.Text;
using System.Threading.Tasks;
using Akka.Actor;
using Akka.Cluster;
using Akka.Configuration;
using Akka.DistributedData;
using Akka.Remote.Configuration;
using Akka.Serialization;
using Akka.TestKit;
using FluentAssertions;
using Google.Protobuf;
using Xunit;
using Xunit.Abstractions;

namespace Akka.Remote.Tests.Serialization
{
public class BugFix5062Spec: AkkaSpec
{
private static Config DDataConfig => ConfigurationFactory.ParseString($@"
akka.actor {{
serializers {{
dummyWithManifest = ""{typeof(DummySerializerWithStringManifest).AssemblyQualifiedName}""
}}
serialization-bindings {{
""{typeof(SomeMessage).AssemblyQualifiedName}"" = dummyWithManifest
}}
serialization-identifiers {{
""{typeof(DummySerializerWithStringManifest).AssemblyQualifiedName}"" = 13
}}
}}")
.WithFallback(RemoteConfigFactory.Default())
.WithFallback(DistributedData.DistributedData.DefaultConfig());

public BugFix5062Spec(ITestOutputHelper output) : base(output, DDataConfig)
{ }

[Fact]
public void Failed_serialization_should_give_proper_exception_message()
{
var node1 = new UniqueAddress(new Address("akka.tcp", "Sys", "localhost", 2551), 1);
var payload = new LWWRegister<SomeMessage>(node1, new SomeMessage());

var message = new ActorSelectionMessage(
payload,
new SelectionPathElement[] { new SelectChildName("dummy") },
true);

var serialized = MessageSerializer.Serialize((ExtendedActorSystem)Sys, node1.Address, message);

var o = new object();
var ex = o.Invoking(s => MessageSerializer.Deserialize((ExtendedActorSystem)Sys, serialized)).Should()
.Throw<SerializationException>()
.WithMessage("Failed to deserialize object with serialization id [6] (manifest []).")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This literally reproduces the error we want to get rid of in #5062

.WithInnerExceptionExactly<SerializationException>()
.WithMessage("Failed to deserialize object with serialization id [11] (manifest [E]).")
.WithInnerExceptionExactly<TargetInvocationException>()
.WithMessage("Exception has been thrown by the target of an invocation.")
.WithInnerExceptionExactly<SerializationException>()
.WithMessage("Failed to deserialize object with serialization id [13] (manifest [SM]).");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your use case and reproduction here is wrong - need to have a scenario where SomeMessage isn't registered at all, not that it fails during deserialization.

The goal of this bug is to unpack the inner exception and throw that - so the user gets a clear error message: that the payload carried by an ActorSelection couldn't be found because of a missing manifest, not that the ActorSelection itself couldn't be deserialized.

}

public class SomeMessage
{
}

public class DummySerializerWithStringManifest : SerializerWithStringManifest
{
public DummySerializerWithStringManifest(ExtendedActorSystem system) : base(system)
{
}

public override byte[] ToBinary(object obj)
{
return Array.Empty<byte>();
}

public override object FromBinary(byte[] bytes, string manifest)
{
throw new NotImplementedException();
}

public override string Manifest(object o)
{
if (o is SomeMessage)
return "SM";
throw new Exception("Unknown object type");
}
}


}
}
5 changes: 5 additions & 0 deletions src/core/Akka/Serialization/Serialization.cs
Expand Up @@ -429,6 +429,11 @@ public object Deserialize(byte[] bytes, int serializerId, string manifest)

return serializer.FromBinary(bytes, type);
}
catch (Exception e)
{
throw new SerializationException(
Aaronontheweb marked this conversation as resolved.
Show resolved Hide resolved
$"Failed to deserialize object with serialization id [{serializerId}] (manifest [{manifest}]).", e);
}
finally
{
Serialization.CurrentTransportInformation = oldInfo;
Expand Down