kryo
kryo copied to clipboard
EsotericSoftware
Add a default registration if missing Class structure when re…
An recommendation for #643 in #684 , give a default registration if missing Class structure when reading
@magro , I reopen this PR for trigger CI rebuild and it has been passed, log is here https://travis-ci.org/github/EsotericSoftware/kryo/builds/687970498, and I don't know why this page show that CI build is still failed.
Not sure either. Maybe rebasing on master would help?
I'd also like to have a test for this, this would also trigger a completely fresh build 😁
The test could use 2 kryo instances, where the 1st kryo instance is configured with a classloader that also "knows" a dynamically generated class. This could be used for inspiration, although it's only working with a modified class: https://github.com/magro/memcached-session-manager/blob/master/kryo-serializer/src/test/java/de/javakaffee/web/msm/serializer/kryo/SerializationChangeTests.java (and here we should use asm instead of javassist).
@maxisvest: If you want to get this merged, please provide a testcase that demonstrates what you are trying to achieve with your change.
@theigl : Yes，I will write the test case later. But the change I was provide is used to the case of transfer data in two services, and those two services rely one DTO(Data transfer Object) with different version. Which means this test case can't run automaticlly in one service because you should change DTO structure manually to complete the test case.
@maxisvest: Thank you! I tried your test-case against the current master and it does not throw any exceptions in my case. Could this somehow depend on the JDK vendor and version used? I'm using OpenJDK 11 and the class is deserialized without any issue after deleting the SubData.
@theigl : I think this is not depended on any special JDK version. Just like deserializition on other class. Or should I add a switch for this feature?
@maxisvest: We first need a test-case that can reliably replicate the issue. Please merge the current master into your branch and make sure that your test-case really fails. Currently, I cannot reproduce the issue.
On master, my bean looks like this after following all the steps in your test-case:
No exception is thrown.
@theigl : Yes, after merge master the issue is disappeared. Because in FieldSerializer will sort cachedFields by field name. It will make message in front of subData and serialize message first. So I simplely rename subData to aSubData. Make sure aSubData serialize first in DataBean. And then run test agian in current master, my issue will be reliably replicated.
@maxisvest: Thanks. I can now reproduce your issue and I think I understand what's going on: When chunked encoding is enabled and Kryo encounters an unknown class, it skips the whole chunk as described in the JavaDoc:
In either case, if the data is skipped and {@link Kryo#setReferences(boolean) references} are enabled, then any references in the skipped data are not read and further deserialization receive the wrong references and fail.
Your fix works by forcing Kryo to read the chunk to a generic object. It solves the problem but could theoretically cause incorrect behavior for other users. We could disable it by default but I'm not sure what we would call the configuration option.
@magro: Do you have an opinion on this?
I think I don't understand the consequences completely, e.g. in combination with references. E.g. I couldn't tell what the documentation for the new setting would be 😉 Having that would help me.
So far I have a slight feeling that this change adds more complexity than value, but that's just a feeling.
Another thought:
In my projects I find it quite naturally to do multi step deployments. E.g. if I want to remove a field from a table/db which is used by multiple instances, at first I change the code to no longer use it (deployment 1) and then I remove the column from the table (deployment 2).
Couldn't the same pattern be applied here? At first add the class so that all parties/services have that class (deployment 1), then use the class in RPCs (deployment 2)?
@magro : Yes, Theoretically upgrade a common class should take multi step as you says. But to our service system may have special situation on depolyment that is sometime services can't deploy at same time. So it cause the differentiation and I try to avoid it.