EsotericSoftware / kryo

Java binary serialization and cloning: fast, efficient, automatic
BSD 3-Clause "New" or "Revised" License
6.21k stars 826 forks source link

Java8 Lambdas cannot be deserialized #215

Closed ikabiljo closed 10 years ago

ikabiljo commented 10 years ago

Deserializing Java8 lambdas fails with ClassNotFoundException: Test$$Lambda$1/1279149968

Example:

  private <T> T kryoSerDeser(T r) throws IOException, ClassNotFoundException {
    Kryo kryo = new Kryo();
    ByteArrayOutputStream baos = new ByteArrayOutputStream();
    try (Output output = new Output(baos)) {
      kryo.writeClassAndObject(output, r);
    }

    try (Input input = new Input((new ByteArrayInputStream(baos.toByteArray())))) {
      return (T) kryo.readClassAndObject(input);
    }
  }

  @Test
  public void testLambdaInKryo() throws IOException, ClassNotFoundException {
    Runnable r = () -> System.out.println("works");
    kryoSerDeser(r).run();
  }

Using Java's standard ObjectOutput/Input streams on lambdas implementing Serializable interface works fine.

romix commented 10 years ago

I can confirm your report. Kryo seems to have a problem with a closure class, because this class seems to be generated by the JDK at runtime. Kryo writes the name of this dynamically generated class into the output stream. Later on, during deserialization, Kryo sees this name and tries to performa a class look-up using it. But it cannot find it.

We need to investigate if it is just a naming convention problem related to lambda classes or if it is a deeper issue.

romix commented 10 years ago

Found a related discussion here: http://openjdk.5641.n7.nabble.com/Lambda-class-names-no-longer-reported-when-listening-for-JVMTI-EVENT-CLASS-FILE-LOAD-events-td174840.html

romix commented 10 years ago

After reading more about lambdas serialization in JDK I got the impression that it would be rather difficult to support it efficiently. One idea to check out would be to declare/cast your lambdas into Serializable and then use Kryo's JavaSerializer to serialize it using standard Java serialization. Could you try it?

BTW, have you tried any other serialization frameworks besides JDK itself? Are there any that are able to serialize lambdas?

ikabiljo commented 10 years ago

First issue is that for lambdas, Class.forName(lambda.getClass().getName()) doesn't work.

And so, in above example, using kryo.setDefaultSerializer(JavaSerializer.class) and casting lambda to Serializable still fails with same issue. But even if that worked, that would bring more trouble as well:

Java Serialization does something specific for lambdas, is it possible to replicate such behavior?

romix commented 10 years ago

Java Serialization does something specific for lambdas, is it possible to replicate such behavior?

This is a big question. Based on what I've learned so far, JDK's Java Serialization does something very specific for lambdas (e.g. it generates special invisible methods for serialization, reconstruction, etc). And it seems to be specific to Oracle JDK. So, IMHO, there is no reliable way to reproduce it outside of the JDK at the moment.

I'd be glad to look into this issue, if I'd be provided with more information about the current implementation of lambda's serialization in the JDK. Then we could try to mimic it, if it is a reasonable effort.

romix commented 10 years ago

OK, I have some good news. I managed to make your example work for the following lambda!

        Runnable r = (Runnable & Serializable) (() -> System.out.println("works"));

Without cast to serializable, I'm not even sure that a usual Java Serialization would work.

I figured out how lambdas are serialized/deserialized by the JDK. In fact it is not as bad as it could be ;-) Now I need to find a way how this could be incorporated into Kryo.

ikabiljo commented 10 years ago

Sounds promising!

@romix - how did you make it work for that lambda?

romix commented 10 years ago

Here is some code to give you an idea:

    private <T> T kryoSerDeser (T r) throws IOException, ClassNotFoundException {
        Kryo kryo = new Kryo();
        kryo.setInstantiatorStrategy(new StdInstantiatorStrategy());
        ByteArrayOutputStream baos = new ByteArrayOutputStream();
        try (Output output = new Output(baos)) {
            kryo.writeClassAndObject(output, r);
        }

        try (Input input = new Input((new ByteArrayInputStream(baos.toByteArray())))) {
            return (T)kryo.readClassAndObject(input);
        }
    }

    @Test
    public void testLambdaInKryo () throws Exception {
        // Casting to Serializable forces java run-time to generate dedicated
        // methods supporting serialization of lambdas, e.g. writeReplace.
        Runnable r = (Runnable & Serializable) (() -> System.out.println("works"));
        Class c = r.getClass();
        Method writeReplace = c.getDeclaredMethod("writeReplace");
        Method readResolve = SerializedLambda.class.getDeclaredMethod("readResolve");
        writeReplace.setAccessible(true);
        readResolve.setAccessible(true);
        // Generate a serializable representation of this lambda
        Object replacement = writeReplace.invoke(r);
        if (replacement instanceof SerializedLambda) {
            SerializedLambda l = (SerializedLambda)replacement;
            // Serialize and deserialize the representation of this lambda
            // Use readResolve to create a real lambda object from this representation
            ((Runnable)(Object)(readResolve.invoke(kryoSerDeser(l)))).run();
        } else
            Assert.fail("Could not serialize lambda");
    }
ikabiljo commented 10 years ago

Very cool. Hoping to see this in Kryo soon :)

romix commented 10 years ago

I have a patch for Kryo almost ready. But there is a small problem:

SerializedLambda is defined only in Java8. If I commit a serializer using it, it will be impossible to compile Kryo with Java6 or Java7, which is not so good... Of course, I could use reflection to completely avoid referencing this class in a static way, but this would make the serializer slower, I guess. And producing a dedicated Kryo JAR for Java8 only because of this single class is also not so nice.

@NathanSweet @magro How should we proceed in such a case? What is the best option?

NathanSweet commented 10 years ago

Use reflection to only register the serializer for Java 8.

Building it should be manageable. The serializer would need to be built for Java 8, but could be included in the JAR that works for Java 6. I don't know if Martin wants to figure out that nasty build stuff though. :)

On Fri, May 9, 2014 at 1:57 PM, romix notifications@github.com wrote:

I have a patch for Kryo almost ready. But there is a small problem:

SerializedLambda is defined only in Java8. If I commit a serializer using it, it will be impossible to compile Kryo with Java6 or Java7, which is not so good... Of course, I could use reflection to completely avoid referencing this class in a static way, but this would make the serializer slower, I guess. And producing a dedicated Kryo JAR for Java8 only because of this single class is also not so nice.

@NathanSweet https://github.com/NathanSweet @magrohttps://github.com/magroHow should we proceed in such a case? What is the best option?

— Reply to this email directly or view it on GitHubhttps://github.com/EsotericSoftware/kryo/issues/215#issuecomment-42658368 .

romix commented 10 years ago

Use reflection to only register the serializer for Java 8.

Yes, sure. That is what was planned.

Building it should be manageable. The serializer would need to be built for Java 8, but could be included in the JAR that works for Java 6.

Yes, I understand this. This is also what we do for Android-specific stuff (or the other way around, this is why Android can load JARs with classes that are Java6 specific and cannot be executed on Android. But this is not a problem as long as these classes are not loaded and invoked ;-)

I don't know if Martin wants to figure out that nasty build stuff though. :)

:-) What I would like to avoid is to require Java8 for building Kryo. Though, if it is built by our own CI systems like Jenkins, it should not be a problem. It may become a problem for users who want to build it on their own, as it would force them to install Java8

NathanSweet commented 10 years ago

People can exclude the Java 8 class from their build if they don't care about it.

On Fri, May 9, 2014 at 4:53 PM, romix notifications@github.com wrote:

Use reflection to only register the serializer for Java 8.

Yes, sure. That is what was planned.

Building it should be manageable. The serializer would need to be built for Java 8, but could be included in the JAR that works for Java 6.

Yes, I understand this. This is also what we do for Android-specific stuff (or the other way around, this is why Android can load JARs with classes that are Java6 specific and cannot be executed on Android. But this is not a problem as long as these classes are not loaded and invoked ;-)

I don't know if Martin wants to figure out that nasty build stuff though. :)

:-) What I would like to avoid is to require Java8 for building Kryo. Though, if it is built by our own CI systems like Jenkins, it should not be a problem. It may become a problem for users who want to build it on their own, as it would force them to install Java8

— Reply to this email directly or view it on GitHubhttps://github.com/EsotericSoftware/kryo/issues/215#issuecomment-42674749 .

magro commented 10 years ago

Probably a separate source path like src/main/java8 or src8/ could be added when built with java 8 using maven profiles. See e.g. http://books.sonatype.com/mvnref-book/reference/profiles-sect-activation.html

I'd prefer this way, because otherwise we'd loose the ability to test against different jdks.

Cheers, Martin Am 09.05.2014 16:53 schrieb "romix" notifications@github.com:

Use reflection to only register the serializer for Java 8.

Yes, sure. That is what was planned.

Building it should be manageable. The serializer would need to be built for Java 8, but could be included in the JAR that works for Java 6.

Yes, I understand this. This is also what we do for Android-specific stuff (or the other way around, this is why Android can load JARs with classes that are Java6 specific and cannot be executed on Android. But this is not a problem as long as these classes are not loaded and invoked ;-)

I don't know if Martin wants to figure out that nasty build stuff though. :)

:-) What I would like to avoid is to require Java8 for building Kryo. Though, if it is built by our own CI systems like Jenkins, it should not be a problem. It may become a problem for users who want to build it on their own, as it would force them to install Java8

— Reply to this email directly or view it on GitHubhttps://github.com/EsotericSoftware/kryo/issues/215#issuecomment-42674749 .

romix commented 10 years ago

Thanks guys! I'll try to follow your advices and report back how it goes.

romix commented 10 years ago

@magro Martin, I tried to implement this with profiles. But my Maven-fu is probably not very strong. I could not explain Maven that all sources under "src" should be compiled using Java 1.5 and all sources under "src8" using Java 1.8 (I also tried to compile certain sources under "src" using 1.5 and other sources under "src" using 1.8, but it does not make it much easier). After many attempts I almost managed to get it working, but then Eclipse could not properly show the project based on such a complicated pom.xml. My feeling is that Eclipse does not like POMs with profiles, etc. I'd be very obliged, if you could help me a bit and may be provide an example, of how this can be done with Maven in a nice way.

magro commented 10 years ago

Hmm, perhaps it's easier to use 2 maven modules, one that provides the java 8 stuff, and kryo itself depending on the java8 module.

Sorry that I respond that late, too many mails got on top...

Cheers, Martin Am 13.05.2014 12:31 schrieb "romix" notifications@github.com:

@magro https://github.com/magro Martin, I tried to implement this with profiles. But my Maven-fu is probably not very strong. I could not explain Maven that all sources under "src" should be compiled using Java 1.5 and all sources under "src8" using Java 1.8 (I also tried to compile certain sources under "src" using 1.5 and other sources under "src" using 1.8, but it does not make it much easier). After many attempts I almost managed to get it working, but then Eclipse could not properly show the project based on such a complicated pom.xml. My feeling is that Eclipse does not like POMs with profiles, etc. I'd be very obliged, if you could help me a bit and may be provide an example, of how this can be done with Maven in a nice way.

— Reply to this email directly or view it on GitHubhttps://github.com/EsotericSoftware/kryo/issues/215#issuecomment-42939557 .

romix commented 10 years ago

@magro Martin, thanks for your advice. But I think making Kryo depend on the java8 module is not such a great idea. Some people may want to use Kryo without it (e.g. on Android or old Java versions). What do you think of putting such a module outside of Kryo, just like it is done for non-default serializers in your "kryo-serializers" project? Then those who need it may include a dependency on this module. In fact, your project could be a good place for it, but then compiling using Java8 would introduce there the same problems as in Kryo, so probbably it should be a separate project. What do you think?

magro commented 10 years ago

@romix kryo-core could specify scope=optional for the dependency on the java8 module. With this users would need to specify the additional dependency on kryo-java8 explicitely.

romix commented 10 years ago

I decided to use reflection to avoid any problems with Maven, etc. It works for me on Java7 and Java8. I cannot commit my JUnit test as it would require Java8 to compile and thus special Maven tweaks. But I tested using closures with and without arguments. Seems to work fine.

romix commented 10 years ago

@ikabiljo Could you test with the latest trunk? This is what I used for testing, based on your original code:

    private <T> T kryoSerDeser (T r) throws IOException, ClassNotFoundException {
        Kryo kryo = new Kryo();
        kryo.setInstantiatorStrategy(new StdInstantiatorStrategy());
        ByteArrayOutputStream baos = new ByteArrayOutputStream();
        try (Output output = new Output(baos)) {
            kryo.writeClassAndObject(output, r);
        }

        try (Input input = new Input((new ByteArrayInputStream(baos.toByteArray())))) {
            return (T)kryo.readClassAndObject(input);
        }
    }

    @Test
    public void testLambdaWithoutArgsInKryo () throws Exception {
        Log.TRACE();
        Runnable r = (Runnable & Serializable) (() -> System.out.println("works"));
        kryoSerDeser(r).run();
    }

    static interface HelloService {
        public void sayHello (String name);
    }

    @Test
    public void testLambdaWithArgsInKryo () throws Exception {
        Log.TRACE();
        HelloService r = (HelloService & Serializable) ((String name) -> System.out.println("It works, " + name));
        kryoSerDeser(r).sayHello("Leo");
    }
ikabiljo commented 10 years ago

@romix - awesome! Tried out with 2.24.1-SNAPSHOT, with both local and remote examples, and it works with no issues!

Is it worth creating a Java issue - so they add some support for classes that don't implement Serializable? (some equivalent of readResolve/writeReplace) That seems like an arbitrary restriction.

On the other hand I am now hitting issue #216 :)

ikabiljo commented 10 years ago

(I didn't want to reference these two tasks...)

ikabiljo commented 10 years ago

Found one issue - lambdas that capture "this" from the outside scope don't seem to work:

Runnable capturingThis = (Runnable & Serializable) () ->
  System.out.println(this);
kryoSerDeser(capturingThis).run();

Fails with "Invalid lambda deserialization". But if they capture it as a reference, it works:

TestKryoWritableObject o = this;
Runnable capturingThisRef = (Runnable & Serializable) () ->
  System.out.println(o);
kryoSerDeser(capturingThisRef).run();
romix commented 10 years ago

@ikabiljo Your last message about serializing lambdas capturing "this" is interesting. The big question is: does it work in Java8 at all? Is it a JVM/JDK or Kryo issue?

Is it worth creating a Java issue - so they add some support for classes that don't implement Serializable? (some equivalent of readResolve/writeReplace) That seems like an arbitrary restriction.

Well, even without lambdas Java requires classes to implement Serializable or Externalizable if you want to serialize them. It is not very likely that they want change this restriction any time soon. But you may give it a try.

romix commented 10 years ago

Found one issue - lambdas that capture "this" from the outside scope don't seem to work

I think that in this case the class of "this" should implement Serializable as well (see http://docs.oracle.com/javase/tutorial/java/javaOO/lambdaexpressions.html#serialization). May be you can check if this is the issue here?

ikabiljo commented 10 years ago

Ah, this is actually bug in Java... This doesn't work:

class A implements Serializable {
  Runnable get() {
    return (Runnable & Serializable) () -> System.out.println(this);
  }
}
javaSerDeser(new A().get()).run();

And this works:

class A implements Serializable {
  Runnable get() {
    A a = this;
    return (Runnable & Serializable) () -> System.out.println(a);
  }
}
javaSerDeser(new A().get()).run();

Using:

static <T> T javaSerDeser(T r) throws IOException, ClassNotFoundException {
  ByteArrayOutputStream baos = new ByteArrayOutputStream();
  try (ObjectOutput oo = new ObjectOutputStream(baos)) {
    oo.writeObject(r);
  }

  try (ObjectInput oi = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))) {
    return (T) oi.readObject();
  }

}

I've submitted a bug report, will attach here when they review and create it. But that means, it is hardly going to be any time soon... Since that seems unrelated, feel free to close this task.

Regarding "Serializable" - this is I think first instance for Kryo where object cannot be serialized as is, but have a constraint on what they need to implement (Serializable interface) - which means if object creation is not in control of the code that does the serialization - nothing can be done. If what readResolve/writeReplace methods do can be replicated in a library - that is probably the best solution. If they have some compile time magic, that is impossible to introspect and replicate at runtime - then it seems reasonable to ask for that kind of magic to be provided for all lambdas, not just Serializable ones. But of course, that will again not be any time soon. On the side note - captured objects already don't need to be Serializable of course - since they are handled with Kryo as a regular field of a regular class (SerializableLambda) - which is great.

romix commented 10 years ago

Ah, this is actually bug in Java...

This is what I suspected. Thanks for checking it!

Regarding "Serializable" - this is I think first instance for Kryo where object cannot be serialized as is, but have a constraint on what they need to implement (Serializable interface) - which means if object creation is not in control of the code that does the serialization - nothing can be done.

Yes. But it seems like it is currently the only way to serialize lambdas as we have to rely on Javas standard serialization in this case. And, as far as I understand, Java serialization for lambdas is only possible when they are declared to implement Serializable. More over, those special methods (readResolve/writeReplace) are generated by a JVM only if a lambda is Serializable :-(

TODO: we may want to check, if it is possible to force generation of those special methods by a JVM, if a closure is casted to a Serializable inside the serializer, i.e. not at the time of closure creation, but later on. If this is the case, then we can have a nice solution.

If they have some compile time magic, that is impossible to introspect and replicate at runtime - then it seems reasonable to ask for that kind of magic to be provided for all lambdas, not just Serializable ones.

Yes, this would make sense. But it will be pretty inefficient, I guess, because it may significantly increase the amount of code generated by JVM for lambdas.

romix commented 10 years ago

I'm closing this task as serialization of lambdas is implemented and supported now to the extent possible at all with the current implementation of Java8/JVM. Should Java8/JVM improve lambda's serialization in the future, we can revive this discussion.

ikabiljo commented 10 years ago

@romix - any chance of a new release, which will include this fix? (I am currently using 2.24.1-SNAPSHOT)

romix commented 10 years ago

I haven't planned a next release yet, as we have not fixed that many bugs or implemented that many new features yet. But if there will be enough demand, we may release earlier. At the moment it looks like there are some issues with ReflectASM being reported by other people. If this is confirmed and ReflectASM fix will be released, we'll need to produce a new Kryo release which will use the updated ReflectASM pretty quickly...

rgilles commented 9 years ago

Hi Kryo, I would like to know if there is any way to serialize lambda that are not casted with a & Serializable. By the way. I did a test on the 3.0.1 and I was oblige to do the following in order to make it works.

    @Test
    public void serializeSerializableLambdaWithKryo() throws Exception {
        Callable<Boolean> doNothing = (Callable<Boolean> & Serializable)(() -> true);
        Kryo kryo = new Kryo();
        kryo.setInstantiatorStrategy(new Kryo.DefaultInstantiatorStrategy(
                new StdInstantiatorStrategy()));
        kryo.register(java.lang.invoke.SerializedLambda.class);
        kryo.register(Class.forName(Kryo.class.getName() + "$Closure"), new ClosureSerializer());
        Callable<Boolean> result = serialize(kryo, doNothing);
        assertThat(result.call(), is(true));
    }

As you can see the Closure class is private into the kryo class. therefore you need to load it manually. Maybe an update of the documentation?

Regards,

Romain.

orionll commented 9 years ago

+1. Please, provide a full example for serializing/deserializing a lambda on the main page.

magro commented 8 years ago

Related: #299