CompatibleSerializer Issue when upgraded to 5.0

[f2014440]

Currently we are using kryo version: "com.esotericsoftware:kryo:5.0.0-RC4" but when I upgraded to "com.esotericsoftware:kryo:5.0.0-RC5" our test cases are failing. These is our serializer configs:

kryoPool = new Pool<Kryo>(true, true) {
      protected Kryo create() {
        Kryo kryo = new Kryo();
        kryo.setRegistrationRequired(false);
        kryo.setReferences(false);
        kryo.setCopyReferences(false);
        kryo.setInstantiatorStrategy(new StdInstantiatorStrategy());
        CompatibleFieldSerializer.CompatibleFieldSerializerConfig config = new CompatibleFieldSerializer.CompatibleFieldSerializerConfig();
        config.setChunkedEncoding(true);
        config.setReadUnknownFieldData(true);
        kryo.setDefaultSerializer(new SerializerFactory.CompatibleFieldSerializerFactory(config));
        return kryo;
      }

We are using compatibleSerializer and I saw that to resolve one of the issue there was a change made in CompatibleSerializer's write method(link: 4ea2a8a). I think this change is failing test cases. In version it used to get specific class but now it takes type which is Java.lang.Object for us. So while writing nested object using compatibleSerializer it's not able to register the nested object in CacheFields and not able to encode it.
This is the object we want to encode and decode:

public class CacheObject {

  @JsonFormat(shape = JsonFormat.Shape.STRING)
  private Date stored;
  private long ttl;
  private Object value;

  public CacheObject() { }

  public CacheObject(Date stored, long ttl, Object value) {
    this.stored = stored;
    this.ttl = ttl;
    this.value = value;
  }

  public Date getStored() {
    return stored;
  }

  public long getTtl() {
    return ttl;
  }

  public Object getValue() {
    return value;
  }
}

For the object which is inside "value" attribute it's not able to initialize CacheFields and that's why not able to write that object.

[theigl]

Thanks for reporting this @f2014440. I can reproduce your issue.

The fix for #663 does indeed cause the problem. We cannot revert the change because it fixes a serious problem that can lead to a JVM crash.

I see two options:

1. Check if field type is Object.class and if so, use the actual value class instead
2. Use the slightly different approach suggested by @isaki in Fix JVM Crash as reported in #663 #670

@magro: Do you have an opinion on this?

[isaki]

If you need me to redo the change I made for #670 please let me know. It was originally rejected, but I can certainly do it again.

[f2014440]

If you need me to redo the change I made for #670 please let me know. It was originally rejected, but I can certainly do it again.

@isaki if you can remove the changes then it would be great help. I am talking about this change: 4ea2a8a

[isaki]

My change had a different approach; it was ultimately rejected and the approach that you are referring to was used instead. The current implementation must remain or be rolled back and my change put in its place to avoid JVM crashes. I am more than happy to replace the current implementation with my original proposal, however that decision is not mine to make. That decision lies with @magro and @NathanSweet.

EDIT: For reference, look at the diff for #670. I believe adding the parts of my change that were not used into the current code base should be easy. I can clean up comments and code style/access (the code likes to directly reference member fields instead of accessors) to match and submit a PR if desired.

[magro]

@isaki TBH, I'm not sure why @NathanSweet didn't want to apply your PR. Do you have an idea?
What do you think of @theigl 's 1st option to check for Object.class and use the actual value class in this case?

[f2014440]

@isaki TBH, I'm not sure why @NathanSweet didn't want to apply your PR. Do you have an idea?
What do you think of @theigl 's 1st option to check for Object.class and use the actual value class in this case?

@magro I can do that but in that case I have to write my own serializer right. Because the logic to check for object.class and use the actual class is in CompatibleSerializer. It is very common to use nested object and if we already have a solution that will take care of both(jvm crash and this issue) then you guys can include this in library.

[theigl]

What do you think of @theigl 's 1st option to check for Object.class and use the actual value class in this case?

@magro: I realized that this is not enough when I wrote a test-case for the issue. It not only fails for Object.class but for all fields declared as super-types or interfaces.

I created a new PR #745 based on the solution proposed by @isaki.

[isaki]

@magro To quote #663

Thanks @isaki for digging into the problem (both problems, the crash and the unit tests) and providing a PR. I'll commit a fix slightly different from the PR (we can fix the unit test differently and I prefer not putting issues in comments, as it adds a lot of noise over time and is easily outdated).

That was the only feedback I had.

EDIT: Formatting.