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

ArrayIndexOutOfBoundsException when serializing generic lambda written in Kotlin #864

Closed
holgerbrandl opened this issue Nov 7, 2021 · 12 comments · Fixed by #951
Closed

Comments

@holgerbrandl
Copy link

Describe the bug
When serializing an object in Kotlin it fails with an ArrayIndexOutOfBoundsException

Exception in thread "main" com.esotericsoftware.kryo.KryoException: java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
Serialization trace:
definition (org.koin.core.definition.BeanDefinition)
	at com.esotericsoftware.kryo.serializers.ReflectField.write(ReflectField.java:101)
	at com.esotericsoftware.kryo.serializers.FieldSerializer.write(FieldSerializer.java:108)
	at com.esotericsoftware.kryo.Kryo.writeClassAndObject(Kryo.java:711)
	at KryoBeanDefinitionKt.main(KryoBeanDefinition.kt:31)
	at KryoBeanDefinitionKt.main(KryoBeanDefinition.kt)
Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
	at com.esotericsoftware.kryo.util.DefaultGenerics.pushTypeVariables(DefaultGenerics.java:116)
	at com.esotericsoftware.kryo.serializers.FieldSerializer.pushTypeVariables(FieldSerializer.java:147)
	at com.esotericsoftware.kryo.serializers.FieldSerializer.write(FieldSerializer.java:102)
	at com.esotericsoftware.kryo.Kryo.writeObject(Kryo.java:642)
	at com.esotericsoftware.kryo.serializers.ReflectField.write(ReflectField.java:70)
	... 4 more

Process finished with exit code 1

To Reproduce
I've created a minimal example in https://github.com/holgerbrandl/kryo-koin. The respective class to reproduce the stacktrace from above is KryoBeanDefinitionKt#main

Environment:

  • OS: Windows 10
  • JDK Version: 11
  • Kryo Version: 5.2.0

Analysis
It seems to be a mismatch between the array sizes of args and hierarchy in


image

I've tried, but did not manage to narrow down the problem any further.

@theigl
Copy link
Collaborator

theigl commented Nov 8, 2021

@holgerbrandl: Thanks for the report and the reproducer. Any chance you can reproduce this issue in plain Java? This would make it much easier for me to look into this.

As a workaround you can disable generics optimization:

kryo.setOptimizedGenerics(false);

This will typically improve serialization speed at the cost of some extra payload bytes.

@holgerbrandl
Copy link
Author

Thanks. I could try, but since I'm consuming this rather complex library https://github.com/InsertKoinIO/koin, I may struggle here. Both because I do not know the internals of koin well enough, and also because I'm not sure if all kotlin constructs can be easily translated to java (although they all compile into jvm bytecode).

I'll will try to use the workaround as well, and will report my results in here. This may take some time, because I need to work around all the missing zero-arg constructors in koin. But since I really would love to kryotize koin, it's worth the effort (cc: @arnaudgiuliani)

@holgerbrandl
Copy link
Author

Quick update: I managed to de/serialize the BeanDefinition with holgerbrandl/kryo-koin@b8152c60 using your workaround (after adding half a dozen serializers to work around missing arg constructors).

I don't know about the implications of setOptimizedGenerics, so I guess the problem is not fully solved.

Note: I've also started kyrotizing koin itself, but for this many more zero-arg constructors need love and care.

@theigl
Copy link
Collaborator

theigl commented Nov 9, 2021

I don't know about the implications of setOptimizedGenerics, so I guess the problem is not fully solved.

The only negative implication is that Kryo doesn't try to use generics information to reduce the serialized payload size. If you use Kryo primarily for speed, then this shouldn't be a problem. In my main project, generics optimization is disabled as well.

Note: I've also started kyrotizing koin itself, but for this many more zero-arg constructors need love and care.

Can't you use Objenesis to deal with zero-arg constructors? (https://github.com/EsotericSoftware/kryo#instantiatorstrategy)

kryo.setInstantiatorStrategy(new DefaultInstantiatorStrategy(new StdInstantiatorStrategy()));

@holgerbrandl
Copy link
Author

Thanks. Using this compound-strategy I could successfully serialize the mentioned koin from above. Thanks for your great support.

@chrisr3
Copy link
Contributor

chrisr3 commented Apr 4, 2023

A simpler way to reproduce this:

class LambdaTest {
    private class Example(private val p: (Long) -> String) {
        constructor() : this({ it.toString() })
    }

    @Test
    fun testLambda() {
        val kryo = Kryo().apply {
            isRegistrationRequired = false
        }

        val example = Example()

        val bytes = Output(1024).use { output ->
            kryo.writeClassAndObject(output, example)
            output.toBytes()
        }

        val deserialized = Input(bytes).use { input ->
            kryo.readClassAndObject(input)
        }

        println(deserialized)
    }
}

What seems to be happening is that the Example.p field has a synthetic type which inherits from kotlin.jvm.internal.Lambda<out R>. This has a single generic parameter R. However, the actual generic arguments for this lambda are Long and String, and so Kryo has more arguments than parameters.... 💥.

@theigl
Copy link
Collaborator

theigl commented Apr 4, 2023

@chrisr3: Thanks a lot for the reproducer!

Do you know how we could add Kotlin support to our test? I'd really like to have a unit test when I fix this issue.

@chrisr3
Copy link
Contributor

chrisr3 commented Apr 4, 2023

Do you know how we could add Kotlin support to our test? I'd really like to have a unit test when I fix this issue.

Well, that unit test only uses Kryo, Kotlin and JUnit 5, so it shouldn't be too difficult. You'd need to add the Kotlin standard library to your tests:

<dependencies>
    <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-stdlib</artifactId>
        <version>${kotlin.version}</version>
        <scope>test</scope>
    </dependency>
</dependencies>

and presumably include the Kotlin Maven plugin to compile it too. I think JetBrains provides more details.


Something like this, perhaps?

@chrisr3
Copy link
Contributor

chrisr3 commented Apr 4, 2023

Here's a "sledge-hammer" fix 😉

diff --git a/src/com/esotericsoftware/kryo/util/DefaultGenerics.java b/src/com/esotericsoftware/kryo/util/DefaultGenerics.java
index eeb2273..953215e 100644
--- a/src/com/esotericsoftware/kryo/util/DefaultGenerics.java
+++ b/src/com/esotericsoftware/kryo/util/DefaultGenerics.java
@@ -94,7 +94,9 @@ public final class DefaultGenerics implements Generics {
        @Override
        public int pushTypeVariables (GenericsHierarchy hierarchy, GenericType[] args) {
                // Do not store type variables if hierarchy is empty or we do not have arguments for all root parameters.
-               if (hierarchy.total == 0 || hierarchy.rootTotal > args.length) return 0;
+               if (hierarchy.total == 0 || hierarchy.rootTotal > args.length || args.length > hierarchy.counts.length) {
+                       return 0;
+               }
 
                int startSize = this.argumentsSize;
 

@theigl
Copy link
Collaborator

theigl commented Apr 5, 2023

Your sledge-hammer fix looks good to me. It's not worse than the other two "fixes" I applied to various generics corner cases. ;)

I will merge it after my PR that adds support for Kotlin tests.

@chrisr3
Copy link
Contributor

chrisr3 commented Apr 5, 2023

I will merge it after my PR that adds support for Kotlin tests.

You might want to replace

println(deserialized)

with some actual assertions first though 😉. I made a similar change to my own PR.

@theigl
Copy link
Collaborator

theigl commented Apr 6, 2023

Merged. Thanks again @chrisr3!

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

Successfully merging a pull request may close this issue.

3 participants