From 91cdcc6752ca20e6ac84d3c1977a853cecec4eee Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Mon, 26 Mar 2018 19:09:03 +0100 Subject: [PATCH] CORDA-1258 - Only register custom serializers once (#2862) * CORDA-1258 - Only register custom serializers once * Review comments * Fix test --- .../core/serialization/SerializationAPI.kt | 2 +- docs/source/changelog.rst | 4 + .../amqp/AMQPSerializationScheme.kt | 15 ++- .../serialization/amqp/SerializerFactory.kt | 2 +- .../ForbiddenLambdaSerializationTests.java | 18 ++-- .../amqp/SerializationSchemaTests.kt | 98 +++++++++++++++++++ 6 files changed, 125 insertions(+), 14 deletions(-) create mode 100644 node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt diff --git a/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt b/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt index b8e90fc8c2..dd94c74898 100644 --- a/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt +++ b/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 } } /** diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 6ba192aa06..e8641a5c49 100644 --- a/docs/source/changelog.rst +++ b/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 diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt index 96d01fa287..4d0ca7b924 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt +++ b/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) : SerializationScheme { +open class SerializerFactoryFactory { + open fun make(context: SerializationContext) = + SerializerFactory(context.whitelist, context.deserializationClassLoader) +} +abstract class AbstractAMQPSerializationScheme( + val cordappLoader: List, + 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) 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 deserialize(byteSequence: ByteSequence, clazz: Class, context: SerializationContext): T { diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt index 94f319f697..9041062452 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt +++ b/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) { + open fun register(customSerializer: CustomSerializer) { if (!serializersByDescriptor.containsKey(customSerializer.typeDescriptor)) { customSerializers += customSerializer serializersByDescriptor[customSerializer.typeDescriptor] = customSerializer diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java index 123bf60e0f..c798548987 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java +++ b/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 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 { - EnumSet contexts = EnumSet.complementOf(EnumSet.of(SerializationContext.UseCase.Checkpoint)); - + public final void serialization_fails_for_serializable_java_lambdas() { 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 target = (Callable & 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 { - EnumSet contexts = EnumSet.complementOf(EnumSet.of(SerializationContext.UseCase.Checkpoint)); - + public final void serialization_fails_for_not_serializable_java_lambdas() { 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 target = () -> value; diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt new file mode 100644 index 0000000000..066278b2c7 --- /dev/null +++ b/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) { + ++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 .serialize. +// Returns the testing scheme we created above that wraps the testing factory. +class TestSerializationFactory : SerializationFactory() { + private val scheme = AMQPTestSerializationScheme() + + override fun deserialize( + byteSequence: ByteSequence, + clazz: Class, context: + SerializationContext + ): T { + throw UnsupportedOperationException() + } + + override fun deserializeWithCompatibleContext( + byteSequence: ByteSequence, + clazz: Class, + context: SerializationContext + ): ObjectWithCompatibleContext { + throw UnsupportedOperationException() + } + + override fun 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) + } +} \ No newline at end of file