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
CompatibleFieldSerializer fails with IndexOutOfBoundsException on field removal. #286
Comments
Having examined the code further, I've managed to come up with a simple code to reproduce the problem. Eclipse/Maven project can be downloaded here: https://copy.com/4Ez9nMNBujRP4ECb Run TestKryoMain, observe that result doesn't match expectation: ddd value should be 'bbb', got: ccc Uncomment bbb field in TestKryoData and run again for comparison. This one is actually much worse -- it doesn't fail, but deserialized data is actually WRONG. The serialized data for this was created with the following field configuration in TestKryoData:
|
Please don't post a link to a zip file. I don't see a problem with this test. Please give step by step instructions On Tue, Feb 17, 2015 at 4:24 PM, solf notifications@github.com wrote:
|
To easily see the problem you need a kryo-serialized file that is inside that archive. If you want to reproduce this using the code -- uncomment ddd declaration: Run it to obtain the serialized file. Revert all the changes and run it again -- observe that ddd value isn't 'bbb' but is, in fact 'ccc'. As an aside, code in pastebin is wrong in the sense that it says that value should be 'ddd' -- it should be 'bbb'. P.S. I don't see an easy way to reproduce the problem with a single java run if we don't have serialized data already (as in the archive that I linked) -- because it requires different Java files structure for write & read. P.P.S. What is the problem with zip files? |
A zip file is unnecessary when it can be done in a single .java file. It's I tested the code I posted. Eg:
I don't see an issue. Please provide repro steps similar to the above that On Wed, Feb 18, 2015 at 12:17 AM, solf notifications@github.com wrote:
|
Start with this: Then run this: Observe that deserialization result is wrong (under other circumstances the same problem would cause deserialization to crash). Uncomment bbb field and run again -- observe that deserialization is fine now. This is caused by the fact that skipping field during deserialization leads to wrong indices in reference resolver (and additionally skipped object is not deserialized which will lead to other problems if it is referenced later on in the stream). You can skip the first file if you take serialization data from the project I've posted. |
Thanks, I see the issue now. Here is the problem: Field If we deserialized the bytes for You'd think it might be possible to store null in the references so the CompatibleFieldSerializer could store field type information or whether a I don't see a solution to the problem. I've added javadocs to On Wed, Feb 18, 2015 at 2:18 PM, solf notifications@github.com wrote:
|
I don't think this is impossible to solve. The first thing that comes to mind -- if there's a case of an object that needs deserialization, but there's no corresponding field (e.g. as in the case of bbb above), then store serialized object data somewhere (it should be possible since obviously it is possible to skip it). If there's later reference to it, it will also have class/type information and thus it should be possible to de-serialize stored object data at that point. Basically something like lazy de-serialization. As to the question of reference index shift -- I can't say without digging more into kryo code, but I would imagine it should be possible to determine whether we have stored reference or stored object data at the given point? My assumption is that references are created when there's object data at that point. How does it sound? P.S. Field addition/removal is the original reason I've chosen kryo for my needs. If this is unsupported it removes (for me) the principal reason for using kryo. |
On Wed, Feb 18, 2015 at 3:53 PM, solf notifications@github.com wrote:
P.S. Field addition/removal is the original reason I've chosen kryo for my
CompatibleFieldSerializer has to use chunked encoding to skip bytes. This |
I'm a bit busy at the moment and couldn't get back to this yet. Could you please elaborate though? If, as you say, it is impossible to determine whether at a given point we have a stored reference or a stored object (so e.g. we can't figure out whether reference resolver should increment an index), what would be the point of adding 'lazy deserialization' I proposed? It still wouldn't work, would it? |
On Mon, Feb 23, 2015 at 3:33 PM, solf notifications@github.com wrote:
|
So I've spent more time on this. So I came with an idea to stuff obsolete fields declaration into a static inner class. That way they don't eat any memory in original class instances, don't clutter original class (aside from the inner class itself), and are not serialized when doing serialization. And since field information is still available, deserialization can actually work. The patch for this is very simple and is localized almost entirely to CompatibleFieldSerializer -- I only had to add a single field to CachedField class. Patch for 2.4.0 can be found here: https://copy.com/GKX9bFipYQytSTbM Please consider adding something similar to the kryo distribution. The performance impact of this is basically nil when feature is not used. When obsolete data is actually present during de-serialization -- then there's additional object instantiation for each instance with obsolete fields. There are a couple of obvious improvements to be considered:
For reference, here's how data class in test code should look like with this support:
|
Hi, all. 'Cause I have same issue when we are using Kryo CompatibleFieldSerializer to handle the serialization between memcached data and java bean, I tried to make some patch for it. I've tried many things and it seems I've made one working code. This class is extending CompatibleFieldSerializer and overriding read(). Even thought I've fixed the bug about IndexOutOfBoundsException in CompatibleFieldSerializer#read(), I've found that mapping the value from Input to Field fails because of wrong chunk reading calculation. And DummyCachedField is the trick to cover such a failure. Simple case it was but, different from the javadoc on CompatibleFieldSerializer, this code seems to be working with Kryo.setReference(true) option. I hope this class can be helpful. public static class FieldMappingCompatibleSerializer extends CompatibleFieldSerializer {
} Kryo setting Kryo kryo = new Kryo(); Thanks. |
Further discussion in #291 |
Many kryo versions are affected, I've tried with 3.0.0, 2.24.0, 2.23.1 and more.
Using some rather complex data structures this can happen during deserialization with CompatibleFieldSerializer if I remove (comment out) field in Java class (in particular case it was a simple String field):
(this is using 2.24.0 but line numbers may be off by one or two because of debug output I had to add to track it down)
Unfortunately data model is proprietary so I can share serialized data + java classes.
However I did a bunch of digging around and here's what I found -- it seems that CompatibleFieldSerializer skips de-serialization for missing fields like this: inputChunked.nextChunks();
This works fine EXCEPT when the actual reading code (which is employed if field is present) creates an additional object in MapReferenceResolver. I've added debug code that dumps what is added to ReferenceResolver (format is [id] -> [object]) and here's what happens:
when field is present (deserialisation works fine):
when field is removed (deserialization crashes later on):
Because of the field skip, object ID 7128 (which should be '1' according to 'normal' deserialization process) is skipped and causes all subsequent IDs to shift -- which ultimately causes deserialization to fail.
These conditions don't happen every time -- not every skipped field is going to cause this failure. But it does happen in my case (not on the first skipped field instance, but eventually).
And finally I've added stack trace dump -- in case of 7128 this is the stack trace that leads to reference resolver state being modified:
This problem is affecting my production code (i.e. I can't remove obsolete fields) so if there's anything else I can do to help tracking this down (without divulging proprietary info) -- do let me know please, I'll do the best I can to help.
The text was updated successfully, but these errors were encountered: