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

treat some deserialization errors as transient in remoting #3782

Merged
merged 8 commits into from Jun 26, 2019

Conversation

@zbynek001
Copy link
Contributor

commented May 9, 2019

treat some deserialization errors as transient in remoting

Migrated from
akka/akka#20641
akka/akka#24910

@zbynek001 zbynek001 changed the title [WIP] treat some deserialization errors as transient in remoting treat some deserialization errors as transient in remoting May 9, 2019

@Aaronontheweb

This comment has been minimized.

Copy link
Member

commented May 14, 2019

this is a very welcome change I've been wanting to add myself. Thanks for handling it! I'll review it as soon as I can.

@Aaronontheweb Aaronontheweb added this to the 1.4.0 milestone May 14, 2019

catch NotSerializableException from deserialization, Migrated from #2…
…0641

* to be able to introduce new messages and still support rolling upgrades,
  i.e. a cluster of mixed versions
* note that it's only catching NotSerializableException, which we already
  use for unknown serializer ids and class manifests
* note that it is not catching for system messages, since that could result
  in infinite resending

Do not tear down connections on IllegalArgumentException from serializer, Migrated from #24910

@zbynek001 zbynek001 force-pushed the zbynek001:drop-association branch from a9b670d to a5ad71c May 17, 2019

@Aaronontheweb
Copy link
Member

left a comment

LGTM - great change that should make life a lot better for Akka.Remote users.

@@ -157,7 +158,7 @@ public override object FromBinary(byte[] bytes, string manifest)
if (_fromBinaryMap.TryGetValue(manifest, out var factory))
return factory(bytes);

throw new ArgumentException($"Unimplemented deserialization of message with manifest [{manifest}] in [{this.GetType()}]");
throw new SerializationException($"Unimplemented deserialization of message with manifest [{manifest}] in [{this.GetType()}]");

This comment has been minimized.

Copy link
@Aaronontheweb


[Fact]
public void The_transport_must_stay_alive_after_a_transient_exception_from_the_serializer()

This comment has been minimized.

Copy link
@Aaronontheweb
ackAndMessage.MessageOption.RecipientAddress,
ackAndMessage.MessageOption.SerializedMessage,
ackAndMessage.MessageOption.SenderOptional);
try

This comment has been minimized.

Copy link
@Aaronontheweb
/// if the manifest is unknown.This makes it possible to introduce new message
/// types and send them to nodes that don't know about them. This is typically
/// needed when performing rolling upgrades, i.e.running a cluster with mixed
/// versions for while. <see cref="SerializationException"/> is treated as a transient

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb May 21, 2019

Member

Comment is helpful and descriptive

Aaronontheweb added some commits May 22, 2019

@Aaronontheweb Aaronontheweb merged commit 2fb49c5 into akkadotnet:dev Jun 26, 2019

3 of 8 checks passed

PR Validation Build #2019.6.19.3 had test failures
Details
PR Validation (.NET Core Unit Tests (Linux)) .NET Core Unit Tests (Linux) failed
Details
PR Validation (.NET Core Unit Tests (Windows)) .NET Core Unit Tests (Windows) failed
Details
PR Validation (.NET Framework Multi-Node Tests (Windows)) .NET Framework Multi-Node Tests (Windows) failed
Details
PR Validation (.NET Framework Unit Tests (Windows)) .NET Framework Unit Tests (Windows) failed
Details
PR Validation (.NET Core Multi-Node Tests (Windows)) .NET Core Multi-Node Tests (Windows) succeeded
Details
PR Validation (NuGet Pack) NuGet Pack succeeded
Details
PR Validation (Windows Build) Windows Build succeeded
Details

madmonkey added a commit to madmonkey/akka.net that referenced this pull request Jul 12, 2019

catch NotSerializableException from deserialization, Migrated from #2…
…0641 (akkadotnet#3782)

* to be able to introduce new messages and still support rolling upgrades,
  i.e. a cluster of mixed versions
* note that it's only catching NotSerializableException, which we already
  use for unknown serializer ids and class manifests
* note that it is not catching for system messages, since that could result
  in infinite resending

Do not tear down connections on IllegalArgumentException from serializer, Migrated from #24910

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Jul 21, 2019

catch NotSerializableException from deserialization, Migrated from #2…
…0641 (akkadotnet#3782)

* to be able to introduce new messages and still support rolling upgrades,
  i.e. a cluster of mixed versions
* note that it's only catching NotSerializableException, which we already
  use for unknown serializer ids and class manifests
* note that it is not catching for system messages, since that could result
  in infinite resending

Do not tear down connections on IllegalArgumentException from serializer, Migrated from #24910

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Jul 26, 2019

catch NotSerializableException from deserialization, Migrated from #2…
…0641 (akkadotnet#3782)

* to be able to introduce new messages and still support rolling upgrades,
  i.e. a cluster of mixed versions
* note that it's only catching NotSerializableException, which we already
  use for unknown serializer ids and class manifests
* note that it is not catching for system messages, since that could result
  in infinite resending

Do not tear down connections on IllegalArgumentException from serializer, Migrated from #24910
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.