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

Java8 Lambdas cannot be deserialized #215

Closed
ikabiljo opened this issue May 3, 2014 · 34 comments
Closed

Java8 Lambdas cannot be deserialized #215

ikabiljo opened this issue May 3, 2014 · 34 comments

Comments

@ikabiljo
Copy link

ikabiljo commented May 3, 2014

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
Copy link
Collaborator

romix commented May 4, 2014

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
Copy link
Collaborator

romix commented May 4, 2014

@romix
Copy link
Collaborator

romix commented May 4, 2014

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
Copy link
Author

ikabiljo commented May 4, 2014

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:

  • what to do with fields lambda captures? Using Java Serialization for them would not be great (for example same object captured by two lambdas would be deserialized as two copies of the object)
  • not sure how to specify use of JavaSerializer only for lambdas

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

@romix
Copy link
Collaborator

romix commented May 4, 2014

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
Copy link
Collaborator

romix commented May 4, 2014

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
Copy link
Author

ikabiljo commented May 7, 2014

Sounds promising!

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

@romix
Copy link
Collaborator

romix commented May 7, 2014

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
Copy link
Author

ikabiljo commented May 8, 2014

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

@romix
Copy link
Collaborator

romix commented May 9, 2014

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
Copy link
Member

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 [email protected] 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:/NathanSweet @magrohttps:/magroHow should we proceed in such a case? What is the best option?


Reply to this email directly or view it on GitHubhttps://issues/215#issuecomment-42658368
.

@romix
Copy link
Collaborator

romix commented May 9, 2014

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
Copy link
Member

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 [email protected] 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://issues/215#issuecomment-42674749
.

@magro
Copy link
Collaborator

magro commented May 9, 2014

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" [email protected]:

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://issues/215#issuecomment-42674749
.

@romix
Copy link
Collaborator

romix commented May 9, 2014

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

@romix
Copy link
Collaborator

romix commented May 13, 2014

@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
Copy link
Collaborator

magro commented May 14, 2014

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" [email protected]:

@magro https:/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://issues/215#issuecomment-42939557
.

@romix
Copy link
Collaborator

romix commented May 15, 2014

@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
Copy link
Collaborator

magro commented May 15, 2014

@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
Copy link
Collaborator

romix commented May 16, 2014

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
Copy link
Collaborator

romix commented May 16, 2014

@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
Copy link
Author

@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
Copy link
Author

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

@ikabiljo
Copy link
Author

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
Copy link
Collaborator

romix commented May 16, 2014

@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
Copy link
Collaborator

romix commented May 16, 2014

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
Copy link
Author

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
Copy link
Collaborator

romix commented May 17, 2014

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
Copy link
Collaborator

romix commented May 19, 2014

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.

@romix romix closed this as completed May 19, 2014
@ikabiljo
Copy link
Author

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

@romix
Copy link
Collaborator

romix commented Jul 17, 2014

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
Copy link

rgilles commented Jun 17, 2015

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
Copy link

orionll commented Jun 30, 2015

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

@magro
Copy link
Collaborator

magro commented Mar 29, 2016

Related: #299

magro added a commit to magro/kryo that referenced this issue Mar 29, 2016
Closure is moved to ClosureSerializer because it's tightly coupled
and this makes the connection more obvious.

This also renames `Kryo.isClousre` to `isClosure`, assuming that it
was not used until now it's ignored regarding binary compatibility.

Fixes EsotericSoftware#299, refs EsotericSoftware#215
magro added a commit to magro/kryo that referenced this issue Mar 29, 2016
Closure is moved to ClosureSerializer because it's tightly coupled
and this makes the connection more obvious.

This also renames `Kryo.isClousre` to `isClosure`, assuming that it
was not used until now it's ignored regarding binary compatibility.

Fixes EsotericSoftware#299, refs EsotericSoftware#215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants