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

Use short assembly name during serialization to avoid versioning problems #28

Merged
merged 1 commit into from
Feb 2, 2017

Conversation

nvivo
Copy link
Contributor

@nvivo nvivo commented Feb 1, 2017

The serializer appears to be built to use short names of types, but the code wasn't implemented this way yet.

This patch makes the serializer write the type name as FullName, Assembly instead of FullName, Assembly, Version=..., PublicKey=.... This fixes issues where deserializer finds a different version of the assembly on the other side.

The reason for this patch is that I'm currently having problems using DistributedPubSub, because Akka.NET depends on a slightly older version of System.Collections.Immutable than some other libraries. As serialized objects carry assembly qualified names, a node with different version of a library throw exceptions trying to decode messages exchanged by the mediator as it cannot find the exact version of the assembly the message was serialized with.

@nvivo
Copy link
Contributor Author

nvivo commented Feb 1, 2017

I just added some code to serialize generic types correctly as well, so generic parameters don't carry version information.

System.Collections.Generic.List`1[[System.Int32, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]], mscorlib

is now serialized as

System.Collections.Generic.List`1[[System.Int32, mscorlib]], mscorlib

This all can be cached for optimization, but for now it just needs to work.

@nvivo nvivo force-pushed the fix-shorname-serialization branch from 6cfdb05 to 089abdf Compare February 2, 2017 12:30
@nvivo
Copy link
Contributor Author

nvivo commented Feb 2, 2017

Rebased.

@Horusiath Horusiath merged commit 04909b1 into akkadotnet:dev Feb 2, 2017
@Aaronontheweb
Copy link
Member

@nvivo @Horusiath would this break wire format compatibility with earlier versions of the serializer by any chance?

{
var res = shortName.Replace(",%core%", CoreAssemblyName);
return res;
var assembly = AppDomain.CurrentDomain.GetAssemblies().FirstOrDefault(a => String.Equals(a.GetName().Name, name.Name, StringComparison.OrdinalIgnoreCase));
Copy link
Contributor

Choose a reason for hiding this comment

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

Retrieving all assemblies everytime we need to load type from string looks bad for performance. We need to measure how this affects the speed to the serializer.

@Horusiath
Copy link
Contributor

Horusiath commented Feb 3, 2017

@Aaronontheweb this for sure will change binary format (i.e. payload new Dictionary<byte, string> {{0, "z"}, {255, "z"}, {3, null}} in my test has been reduced in size from 205 to 177 bytes, effectively reducing manifest size to 105 bytes). However I think, that it shouldn't break backward compatibility.

@nvivo after merging I've seen, that you're basically using LINQ to lookup for a correct assembly by name, loading them all each time. I'm not sure if this is a good idea - maybe we should cache those assemblies by name somewhere?

Also we should probably have a test for binary backward compatiblity - example: have a static file in tests containing a test data payload serialized using Hyperion v0.9.2. The test should load content of that file and make sure, that deserialized instance has all required fields filed (to make test more reasurring, this data structure could contain a custom type from not standard assembly i.e. hyperion test project itself).

@nvivo
Copy link
Contributor Author

nvivo commented Feb 3, 2017

@Aaronontheweb
This might break compatibility because the previous versions wasn't handling type versions at all, it just had comments that this should be done later here and here. Also, mscorlib types were sent with a %core% placeholder. Looks like it was there just to make it work.

But, even if this breaks compatibility, I'm pretty sure this is the right thing to do otherwise this protocol won't work in real world. It's lucky if anyone is using this outside of local testing. For signed libraries, any minor difference will make the deserializer explode. In my case, simply referencing asp.net core was enough to get a newer version of Collections.Immutable and break DistributedPubSub, as it uses immutables to exchange deltas.

@Horusiath as for the speed, I agree this is not the most performant. I was simply doing what needs to be done first. Do you think a using a ConcurrentDictionary as cache should be enough? I could try implementing one in front of these methods.

@nvivo
Copy link
Contributor Author

nvivo commented Feb 3, 2017

Just to clarify the compatibility break:

Old version sends a message for type System.String, %core%, current version shouldn't find the type.
New version sends a message using System.Collections.List[System.String, mscorlib], System.Collections and the previous version won't be to resolve the assemblies without an assemblyResolver function to find the assemby without the version.

Adding code to check for %core% in every lookup just to maintain half compatibility with a pre-release version is not good as well. It will probably degrade performance and will only solve half of the problem.

@Aaronontheweb
Copy link
Member

But, even if this breaks compatibility, I'm pretty sure this is the right thing to do otherwise this protocol won't work in real world. It's lucky if anyone is using this outside of local testing.

Yep, if that's true then I agree. But the reason why I ask is: it's our job to know for certain if this will or will not break binary compatibility, and if it does we have to explicitly communicate that to the end users and try to offer an upgrade path. Can't let it be a surprise. Know what I mean?

@nvivo
Copy link
Contributor Author

nvivo commented Feb 9, 2017

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants