-
Notifications
You must be signed in to change notification settings - Fork 1k
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 serialization verification problem with Akka.IO messages #4974
Changes from 1 commit
6e3dacb
b340a79
bed4022
f1b4440
cf23a2d
bbcb132
aae8b4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,8 @@ class AckWithValue : Tcp.Event | |
|
||
public TcpIntegrationSpec(ITestOutputHelper output) | ||
: base($@"akka.loglevel = DEBUG | ||
akka.actor.serialize-creators = on | ||
akka.actor.serialize-messages = on | ||
akka.io.tcp.trace-logging = true | ||
akka.io.tcp.write-commands-queue-max-size = {InternalConnectionActorMaxQueueSize}", output: output) | ||
{ } | ||
|
@@ -190,7 +192,7 @@ public void The_TCP_transport_implementation_should_properly_support_connecting_ | |
var targetAddress = new DnsEndPoint("localhost", boundMsg.LocalAddress.AsInstanceOf<IPEndPoint>().Port); | ||
var clientHandler = CreateTestProbe(); | ||
Sys.Tcp().Tell(new Tcp.Connect(targetAddress), clientHandler); | ||
clientHandler.ExpectMsg<Tcp.Connected>(TimeSpan.FromMinutes(10)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A socket shouldn't take 10 minutes to connect |
||
clientHandler.ExpectMsg<Tcp.Connected>(TimeSpan.FromSeconds(3)); | ||
var clientEp = clientHandler.Sender; | ||
clientEp.Tell(new Tcp.Register(clientHandler)); | ||
serverHandler.ExpectMsg<Tcp.Connected>(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,12 +19,14 @@ namespace Akka.Tests.IO | |
public class TcpListenerSpec : AkkaSpec | ||
{ | ||
public TcpListenerSpec() | ||
: base(@"akka.io.tcp.register-timeout = 500ms | ||
: base(@" | ||
akka.actor.serialize-creators = on | ||
akka.actor.serialize-messages = on | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this here so we can test similar problems in the future |
||
akka.io.tcp.register-timeout = 500ms | ||
akka.io.tcp.max-received-message-size = 1024 | ||
akka.io.tcp.direct-buffer-size = 512 | ||
akka.actor.serialize-creators = on | ||
akka.io.tcp.batch-accept-limit = 2 | ||
") | ||
akka.io.tcp.batch-accept-limit = 2") | ||
{ } | ||
|
||
[Fact] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,8 @@ public class UdpConnectedIntegrationSpec : AkkaSpec | |
|
||
public UdpConnectedIntegrationSpec(ITestOutputHelper output) | ||
: base(@" | ||
akka.actor.serialize-creators = on | ||
akka.actor.serialize-messages = on | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this here so we can test similar problems in the future |
||
akka.io.udp-connected.nr-of-selectors = 1 | ||
akka.io.udp-connected.direct-buffer-pool-limit = 100 | ||
akka.io.udp-connected.direct-buffer-size = 1024 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,8 @@ public class UdpIntegrationSpec : AkkaSpec | |
|
||
public UdpIntegrationSpec(ITestOutputHelper output) | ||
: base(@" | ||
akka.actor.serialize-creators = on | ||
akka.actor.serialize-messages = on | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this here so we can test similar problems in the future |
||
akka.io.udp.max-channels = unlimited | ||
akka.io.udp.nr-of-selectors = 1 | ||
akka.io.udp.direct-buffer-pool-limit = 100 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,17 +13,21 @@ | |
using Akka.IO; | ||
using Akka.TestKit; | ||
using Xunit; | ||
using Xunit.Abstractions; | ||
using UdpListener = Akka.IO.UdpListener; | ||
|
||
namespace Akka.Tests.IO | ||
{ | ||
public class UdpListenerSpec : AkkaSpec | ||
{ | ||
public UdpListenerSpec() | ||
: base(@"akka.io.udp.max-channels = unlimited | ||
public UdpListenerSpec(ITestOutputHelper output) | ||
: base(@" | ||
akka.actor.serialize-creators = on | ||
akka.actor.serialize-messages = on | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this here so we can test similar problems in the future |
||
akka.io.udp.max-channels = unlimited | ||
akka.io.udp.nr-of-selectors = 1 | ||
akka.io.udp.direct-buffer-pool-limit = 100 | ||
akka.io.udp.direct-buffer-size = 1024") | ||
akka.io.udp.direct-buffer-size = 1024", output) | ||
{ } | ||
|
||
[Fact] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,10 @@ | |
using System.Collections.Immutable; | ||
using System.Text; | ||
using System.Runtime.CompilerServices; | ||
using System.Runtime.Serialization; | ||
using System.Threading; | ||
using Akka.Actor.Internal; | ||
using Akka.Dispatch.SysMsg; | ||
using Akka.Serialization; | ||
using Akka.Util; | ||
using Akka.Util.Internal; | ||
|
@@ -452,6 +454,7 @@ private IInternalActorRef MakeChild(Props props, string name, bool async, bool s | |
if (_systemImpl.Settings.SerializeAllCreators && !systemService && !(props.Deploy.Scope is LocalScope)) | ||
{ | ||
var oldInfo = Serialization.Serialization.CurrentTransportInformation; | ||
object propArgument = null; | ||
try | ||
{ | ||
if (oldInfo == null) | ||
|
@@ -465,17 +468,24 @@ private IInternalActorRef MakeChild(Props props, string name, bool async, bool s | |
{ | ||
if (argument != null && !(argument is INoSerializationVerificationNeeded)) | ||
{ | ||
propArgument = argument; | ||
var serializer = ser.FindSerializerFor(argument); | ||
var bytes = serializer.ToBinary(argument); | ||
var ms = Serialization.Serialization.ManifestFor(serializer, argument); | ||
if(ser.Deserialize(bytes, serializer.Identifier, ms) == null) | ||
if (ser.Deserialize(bytes, serializer.Identifier, ms) == null) | ||
throw new ArgumentException( | ||
$"Pre-creation serialization check failed at [${_self.Path}/{name}]", | ||
nameof(name)); | ||
$"Pre-creation serialization check failed at [${_self.Path}/{name}]", | ||
nameof(name)); | ||
} | ||
} | ||
} | ||
} | ||
catch (Exception e) | ||
{ | ||
throw new SerializationException( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catch and re-throw to add a more meaningful exception message to the error |
||
$"Failed to serialize and deserialize actor props argument of type {propArgument?.GetType()}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's be a good idea to capture the actor type here too |
||
e); | ||
} | ||
finally | ||
{ | ||
Serialization.Serialization.CurrentTransportInformation = oldInfo; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
using Akka.Dispatch.SysMsg; | ||
using Akka.Event; | ||
using System.Reflection; | ||
using System.Runtime.Serialization; | ||
using Akka.Serialization; | ||
using Akka.Util; | ||
using Assert = System.Diagnostics.Debug; | ||
|
@@ -518,6 +519,9 @@ private Envelope SerializeAndDeserialize(Envelope envelope) | |
if (unwrapped is INoSerializationVerificationNeeded) | ||
return envelope; | ||
|
||
if(unwrapped.GetType().Namespace?.StartsWith("System.Net.Sockets") ?? false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed to add this to trap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer it if we just wrapped the |
||
return envelope; | ||
|
||
var deserializedMsg = SerializeAndDeserializePayload(unwrapped); | ||
if (deadLetter != null) | ||
return new Envelope(new DeadLetter(deserializedMsg, deadLetter.Sender, deadLetter.Recipient), envelope.Sender); | ||
|
@@ -540,6 +544,10 @@ private object SerializeAndDeserializePayload(object obj) | |
var manifest = Serialization.Serialization.ManifestFor(serializer, obj); | ||
return _systemImpl.Serialization.Deserialize(bytes, serializer.Identifier, manifest); | ||
} | ||
catch (Exception e) | ||
{ | ||
throw new SerializationException($"Failed to serialize and deserialize payload object {obj.GetType()}", e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catch and re-throw to add a more meaningful exception message to the error |
||
} | ||
finally | ||
{ | ||
Serialization.Serialization.CurrentTransportInformation = oldInfo; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this here so we can test similar problems in the future