Fix in resolving serializer id #3135

Danthar · 2017-09-29T19:22:06Z

Serializer id is now only determined once, upon initialisation. And its
the serializer implementation which dictates where that id comes from.
We no longer assume it always comes from the hocon config.

Danthar · 2017-09-29T19:25:35Z

src/core/Akka/Serialization/Serialization.cs

        {
            _serializers.Add(serializer.Identifier, serializer);
+            _serializerIdByName.Add(name, serializer.Identifier);


Ok so technically, the Identifier is retrieved twice. But since Hocon caching is a thing. I found this to be more expressive then first storing the id in a temp var and using that.

I'd take a look at my previous comment and consider just maintaining two <TKey, Serializer> dictionaries - the ID-based one and the name based one, rather than go name --> id --> serializer you can just go name --> serializer. Does that make sense?

Aaronontheweb · 2017-10-03T12:58:50Z

Going to do a review of this today

Aaronontheweb

The fix is good but needs a bit of API love.

Aaronontheweb · 2017-10-03T15:31:19Z

src/core/Akka/Serialization/Serialization.cs

@@ -91,7 +92,8 @@ public Serialization(ExtendedActorSystem system)
                var serializer = serializerConfig != null
                    ? (Serializer)Activator.CreateInstance(serializerType, system, serializerConfig)
                    : (Serializer)Activator.CreateInstance(serializerType, system);
-                AddSerializer(serializer);
+
+                AddSerializer(kvp.Key, serializer);
                namedSerializers.Add(kvp.Key, serializer);


@Danthar so aren't we doing the same thing with _serializerIdByName and the namedSerializers local here? Why not just use that?

Because we already have a dictionary mapping id => serializer instance. I could have promoted the namedSerializers collection to a private member var. But that would mean we would store a reference to the same instance twice.

Not sure what my rationale was when i was writing this, but i wanted to avoid that. Hence the separate map _serializerIdByName

Aaronontheweb · 2017-10-03T15:32:33Z

src/core/Akka/Serialization/Serialization.cs

@@ -129,10 +131,9 @@ private Serializer GetSerializerByName(string name)
            {
                if (kvp.Key.Equals(name))
                {
-                    var serializerTypeName = kvp.Value.GetString();
-                    var serializerType = Type.GetType(serializerTypeName);
+                    if (!_serializerIdByName.TryGetValue(name, out int serializerId))


I'd remove the serializersConfig line here - no need to read in HOCON if we're just using a cached map.

Yeah your right. The extra check on line 132 serves no purpose.

Aaronontheweb · 2017-10-03T15:33:20Z

src/core/Akka/Serialization/Serialization.cs

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public void AddSerializer(Serializer serializer)
+        public void AddSerializer(string name, Serializer serializer)


This is a breaking API change since we're modifying a public method. IMHO, add a new overload and mark the old one as deprecated. That way we won't have binary compatibility issues on upgrade.

You are correct. However seeing as its being aggressively inlined. Im wondering why it was public in the first place. But ok.

@Danthar

Im wondering why it was public in the first place.

I'm right there with you.

Aaronontheweb · 2017-10-03T15:34:44Z

src/core/Akka/Serialization/Serialization.cs

        {
            _serializers.Add(serializer.Identifier, serializer);
+            _serializerIdByName.Add(name, serializer.Identifier);


I'd take a look at my previous comment and consider just maintaining two <TKey, Serializer> dictionaries - the ID-based one and the name based one, rather than go name --> id --> serializer you can just go name --> serializer. Does that make sense?

Danthar · 2017-10-03T20:15:05Z

src/core/Akka/Serialization/Serialization.cs

-                    var serializerTypeName = kvp.Value.GetString();
-                    var serializerType = Type.GetType(serializerTypeName);
+            if (!_serializersByName.TryGetValue(name, out Serializer serializer))
+                throw new ArgumentException($"Couldn't find serializer for [{name}] make sure you correctly defined.");



I kept this check. Because it still possible to misconfigure for example serializer setting in persistence. Which is not checked during initialization.

Sounds good

Danthar · 2017-10-03T20:15:50Z

src/core/Akka/Serialization/Serialization.cs

-        /// <param name="serializer">TBD</param>
-        /// <returns>TBD</returns>
+        /// <param name="serializer">Serializer instance</param>
+        [Obsolete("No longer supported. Use the AddSerializer(name, serializer) overload instead.", true)]


Left out a "deadline" version here. Or would you rather have i put in something like. will be removed in 1.4 or something ?

This is fine as-is.

Aaronontheweb · 2017-10-03T22:30:40Z

@Danthar looks like a NRE in your new spec

Danthar · 2017-10-04T07:24:38Z

:D its interesting that the linux-netcore/unit-tests does complete.

However another spec failed as well. Ill be fixing both.

alexvaluyskiy · 2017-10-04T07:26:24Z

@Danthar because it does not work #3123

Serializer id is now only determined once, upon initialisation. And its the serializer implementation which dictates where that id comes from. We no longer assume it always comes from the hocon config.

This reverts commit 89a1d6b.

Aaronontheweb added the needs review label Sep 29, 2017

Danthar mentioned this pull request Sep 29, 2017

Akka.Persistence fails to deserialize snapshots after updating to 1.3.1 #3133

Closed

Danthar added the serialization label Sep 29, 2017

Danthar commented Sep 29, 2017

View reviewed changes

Aaronontheweb self-requested a review October 3, 2017 12:58

Aaronontheweb requested changes Oct 3, 2017

View reviewed changes

Danthar commented Oct 3, 2017

View reviewed changes

Danthar and others added 7 commits October 4, 2017 20:09

Fix in resolving serializer id

117c07c

Serializer id is now only determined once, upon initialisation. And its the serializer implementation which dictates where that id comes from. We no longer assume it always comes from the hocon config.

Forgot the nullserializer case.

28715c4

Api Approval

c9d1d97

Updated according to remarks

8f22e74

Added spec

edb9a72

Fix tests

0bbd61e

Merge branch 'dev' into dev

fd980f2

Aaronontheweb approved these changes Oct 10, 2017

View reviewed changes

Aaronontheweb merged commit 89a1d6b into akkadotnet:dev Oct 10, 2017

Aaronontheweb removed the needs review label Oct 10, 2017

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Oct 12, 2017

Revert "Fix in resolving serializer id (akkadotnet#3135)"

4c121c5

This reverts commit 89a1d6b.

Fix in resolving serializer id #3135

Fix in resolving serializer id #3135

Uh oh!

Conversation

Danthar commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aaronontheweb commented Oct 3, 2017

Uh oh!

Aaronontheweb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aaronontheweb commented Oct 3, 2017

Uh oh!

Danthar commented Oct 4, 2017

Uh oh!

alexvaluyskiy commented Oct 4, 2017

Uh oh!

Uh oh!