CORDA-1258 - Only register custom serializers once (#2862)

* CORDA-1258 - Only register custom serializers once

* Review comments

* Fix test

core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt

@@ -182,7 +182,7 @@ interface SerializationContext {
     /**
      * The use case that we are serializing for, since it influences the implementations chosen.
      */
-    enum class UseCase { P2P, RPCServer, RPCClient, Storage, Checkpoint }
+    enum class UseCase { P2P, RPCServer, RPCClient, Storage, Checkpoint, Testing }
 }
 
 /**

docs/source/changelog.rst

@@ -7,6 +7,10 @@ Unreleased
 Here are brief summaries of what's changed between each snapshot release. This includes guidance on how to upgrade code
 from the previous milestone release.
 
+* Fix CORDA-1258. Custom serializers were spuriously registered every time a serialization factory was fetched from the cache rather than
+  only once when it was created. Whilst registering serializers that already exist is essentially a no-op, it's a performance overhead for
+  a very frequent operation that hits a synchronisation point (and is thus flagged as contended by our perfomance suite)
+
 * Update the fast-classpath-scanner dependent library version from 2.0.21 to 2.12.3
 
   .. note:: Whilst this is not the latest version of this library, that being 2.18.1 at time of writing, versions later

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt

@@ -29,8 +29,15 @@ fun SerializerFactory.addToWhitelist(vararg types: Class<*>) {
     }
 }
 
-abstract class AbstractAMQPSerializationScheme(val cordappLoader: List<Cordapp>) : SerializationScheme {
+open class SerializerFactoryFactory {
+    open fun make(context: SerializationContext) =
+        SerializerFactory(context.whitelist, context.deserializationClassLoader)
+}
 
+abstract class AbstractAMQPSerializationScheme(
+        val cordappLoader: List<Cordapp>,
+        val sff : SerializerFactoryFactory = SerializerFactoryFactory()
+) : SerializationScheme {
     // TODO: This method of initialisation for the Whitelist and plugin serializers will have to change
     // when we have per-cordapp contexts and dynamic app reloading but for now it's the easiest way
     companion object {
@@ -116,9 +123,11 @@ abstract class AbstractAMQPSerializationScheme(val cordappLoader: List<Cordapp>)
                     rpcClientSerializerFactory(context)
                 SerializationContext.UseCase.RPCServer ->
                     rpcServerSerializerFactory(context)
-                else -> SerializerFactory(context.whitelist, context.deserializationClassLoader)
+                else -> sff.make(context)
+            }.also {
+                registerCustomSerializers(it)
             }
-        }.also { registerCustomSerializers(it) }
+        }
     }
 
     override fun <T : Any> deserialize(byteSequence: ByteSequence, clazz: Class<T>, context: SerializationContext): T {

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt

@@ -198,7 +198,7 @@ open class SerializerFactory(
      * Register a custom serializer for any type that cannot be serialized or deserialized by the default serializer
      * that expects to find getters and a constructor with a parameter for each property.
      */
-    fun register(customSerializer: CustomSerializer<out Any>) {
+    open fun register(customSerializer: CustomSerializer<out Any>) {
         if (!serializersByDescriptor.containsKey(customSerializer.typeDescriptor)) {
             customSerializers += customSerializer
             serializersByDescriptor[customSerializer.typeDescriptor] = customSerializer

node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java

@@ -4,9 +4,9 @@ import com.google.common.collect.Maps;
 import net.corda.core.serialization.SerializationContext;
 import net.corda.core.serialization.SerializationFactory;
 import net.corda.core.serialization.SerializedBytes;
-import net.corda.testing.core.SerializationEnvironmentRule;
 import net.corda.nodeapi.internal.serialization.kryo.CordaClosureBlacklistSerializer;
 import net.corda.nodeapi.internal.serialization.kryo.KryoSerializationSchemeKt;
+import net.corda.testing.core.SerializationEnvironmentRule;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -19,6 +19,8 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.ThrowableAssert.catchThrowable;
 
 public final class ForbiddenLambdaSerializationTests {
+    private EnumSet<SerializationContext.UseCase> contexts = EnumSet.complementOf(
+            EnumSet.of(SerializationContext.UseCase.Checkpoint, SerializationContext.UseCase.Testing));
     @Rule
     public final SerializationEnvironmentRule testSerialization = new SerializationEnvironmentRule();
     private SerializationFactory factory;
@@ -29,11 +31,10 @@ public final class ForbiddenLambdaSerializationTests {
     }
 
     @Test
-    public final void serialization_fails_for_serializable_java_lambdas() throws Exception {
+    public final void serialization_fails_for_serializable_java_lambdas() {
-        EnumSet<SerializationContext.UseCase> contexts = EnumSet.complementOf(EnumSet.of(SerializationContext.UseCase.Checkpoint));
-
         contexts.forEach(ctx -> {
-            SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(), this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null);
+            SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(),
+                    this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null);
             String value = "Hey";
             Callable<String> target = (Callable<String> & Serializable) () -> value;
 
@@ -51,11 +52,10 @@ public final class ForbiddenLambdaSerializationTests {
 
     @Test
     @SuppressWarnings("unchecked")
-    public final void serialization_fails_for_not_serializable_java_lambdas() throws Exception {
+    public final void serialization_fails_for_not_serializable_java_lambdas() {
-        EnumSet<SerializationContext.UseCase> contexts = EnumSet.complementOf(EnumSet.of(SerializationContext.UseCase.Checkpoint));
-
         contexts.forEach(ctx -> {
-            SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(), this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null);
+            SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(),
+                    this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null);
             String value = "Hey";
             Callable<String> target = () -> value;
 

node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt

@@ -0,0 +1,98 @@
+package net.corda.nodeapi.internal.serialization.amqp
+
+import net.corda.core.serialization.*
+import net.corda.core.utilities.ByteSequence
+import net.corda.nodeapi.internal.serialization.*
+import org.junit.Test
+import kotlin.test.assertEquals
+
+// Make sure all serialization calls in this test don't get stomped on by anything else
+val TESTING_CONTEXT = SerializationContextImpl(amqpMagic,
+        SerializationDefaults.javaClass.classLoader,
+        GlobalTransientClassWhiteList(BuiltInExceptionsWhitelist()),
+        emptyMap(),
+        true,
+        SerializationContext.UseCase.Testing,
+        null)
+
+// Test factory that lets us count the number of serializer registration attempts
+class TestSerializerFactory(
+        wl : ClassWhitelist,
+        cl : ClassLoader
+) : SerializerFactory(wl, cl) {
+    var registerCount = 0
+
+    override fun register(customSerializer: CustomSerializer<out Any>) {
+        ++registerCount
+        return super.register(customSerializer)
+    }
+}
+
+// Instance of our test factory counting registration attempts. Sucks its global, but for testing purposes this
+// is the easiest way of getting access to the object.
+val testFactory = TestSerializerFactory(TESTING_CONTEXT.whitelist, TESTING_CONTEXT.deserializationClassLoader)
+
+// Serializer factory factory, plugs into the SerializationScheme and controls which factory type
+// we make for each use case. For our tests we need to make sure if its the Testing use case we return
+// the global factory object created above that counts registrations.
+class TestSerializerFactoryFactory : SerializerFactoryFactory() {
+    override fun make(context: SerializationContext) =
+            when (context.useCase) {
+                SerializationContext.UseCase.Testing -> testFactory
+                else -> super.make(context)
+            }
+}
+
+class AMQPTestSerializationScheme : AbstractAMQPSerializationScheme(emptyList(), TestSerializerFactoryFactory()) {
+    override fun rpcClientSerializerFactory(context: SerializationContext): SerializerFactory {
+        throw UnsupportedOperationException()
+    }
+
+    override fun rpcServerSerializerFactory(context: SerializationContext): SerializerFactory {
+        throw UnsupportedOperationException()
+    }
+
+    override fun canDeserializeVersion(magic: CordaSerializationMagic, target: SerializationContext.UseCase) = true
+}
+
+// Test SerializationFactory that wraps a serialization scheme that just allows us to call <OBJ>.serialize.
+// Returns the testing scheme we created above that wraps the testing factory.
+class TestSerializationFactory : SerializationFactory() {
+    private val scheme = AMQPTestSerializationScheme()
+
+    override fun <T : Any> deserialize(
+            byteSequence: ByteSequence,
+            clazz: Class<T>, context:
+            SerializationContext
+    ): T {
+        throw UnsupportedOperationException()
+    }
+
+    override fun <T : Any> deserializeWithCompatibleContext(
+            byteSequence: ByteSequence,
+            clazz: Class<T>,
+            context: SerializationContext
+    ): ObjectWithCompatibleContext<T> {
+        throw UnsupportedOperationException()
+    }
+
+    override fun <T : Any> serialize(obj: T, context: SerializationContext) = scheme.serialize(obj, context)
+}
+
+// The actual test
+class SerializationSchemaTests {
+    @Test
+    fun onlyRegisterCustomSerializersOnce() {
+        @CordaSerializable data class C(val a: Int)
+
+        val c = C(1)
+        val testSerializationFactory = TestSerializationFactory()
+        val expectedCustomSerializerCount = 40
+
+        assertEquals (0, testFactory.registerCount)
+        c.serialize (testSerializationFactory, TESTING_CONTEXT)
+        assertEquals (expectedCustomSerializerCount, testFactory.registerCount)
+        c.serialize (testSerializationFactory, TESTING_CONTEXT)
+        assertEquals (expectedCustomSerializerCount, testFactory.registerCount)
+    }
+}