Fix serialize-messages for Akka.Cluster and Akka.Remote #3725
Conversation
Also, disabled serialization verification for internal `PublishChanges`
|
Reproduction spec failed on both Windows and Linux, as is expected. |
| { | ||
| // wait for a singleton cluster to fully form and publish a member up event | ||
| _cluster.Join(_selfAddress); | ||
| var up = ExpectMsg<ClusterEvent.MemberUp>(); |
There was a problem hiding this comment.
The test case here is simple: test that end-to-end, the Akka.Cluster self-join process works and all manner of internal event publication goes off without a hitch. Caught two different major serialization errors with this and fixed both.
There was a problem hiding this comment.
If the MemberUp event makes it all the way to this actor, we can safely assume that the messages necessary to run Akka.Cluster's internals made it intact.
| /// <see cref="INoSerializationVerificationNeeded"/> is not explicitly used on the JVM, | ||
| /// but without it we run into serialization issues via https://github.com/akkadotnet/akka.net/issues/3724 | ||
| /// </remarks> | ||
| private interface IPublishMessage : INoSerializationVerificationNeeded { } |
There was a problem hiding this comment.
First fix: made it so IPublishMessage doesn't get checked by akka.actor.serialize-messages - did this because designating a single Gossip class constructor with JsonConstructor wasn't going to work due to the fact that we make use of all three different constructors in different situations. The JVM gets away with this by using the built-in Serializable trait in Scala, which is roughly equivalent to our binary formatter, which .NET no longer has in .NET Core. So, I figured this would be the best substitute here.
There was a problem hiding this comment.
Relevant link on Scala behavior: https://alvinalexander.com/scala/how-to-use-serialization-in-scala-serializable-trait
| /// <param name="status">The status of this member.</param> | ||
| /// <param name="roles">The roles for this member. Can be empty.</param> | ||
| [JsonConstructor] | ||
| internal Member(UniqueAddress uniqueAddress, int upNumber, MemberStatus status, IEnumerable<string> roles) |
There was a problem hiding this comment.
The issue with serializing the Member class is an old, but familiar one: JSON.NET is unable to serialize the System.Collections.Immutable.ImmutableHashSet<string> taken for the roles parameter on the normal constructor this actor uses. Thus, I created a second internal constructor that just takes an IEnumerable<string> instead and marked that as the JsonConstructor - ideally once Hyperion is done we will no longer need this.
|
Looks like there's still a hard failure on Linux with this. |
|
Failed on Windows too - passes on .NET Framework locally for me, but looks like it doesn't for .NET Core. |
|
I pulled this and it seems to fix my problems in my sample and our actual real code. |
|
@Dan-Albrecht my guess is that it's an issue related to the old version of .NET Core we're running on (1.1.) I haven't had a chance to test that just yet as I've been neck-deep in working on another issue for most of this afternoon (#3414) but I have a plan for getting all of these projects moved onto .NET Core 2.2 pronto as part of an upcoming sprint. Glad to hear the fix is working though - I'll merge this in as part of 1.3.12, somehow :p |
) close akkadotnet#3724 - provided JSON constructor for Member disabled serialization verification for internal `PublishChanges`
) close akkadotnet#3724 - provided JSON constructor for Member disabled serialization verification for internal `PublishChanges`
Working on fixing #3724