[Java] Upgrade 1.9.0 -> 1.11.0 generated code compatibility issue

[alexnixon]

Hi,

Mostly an FYI here, feel free to close if you're uninterested.

I just upgraded my runtime lib from 1.9.0 -> 1.11.0 and the result was breakage because the 1.9.0 generated code doesn't set the vtable_start and vtable_size fields. This results in garbage when decoding.

Out of interest, is it required that the flatc version must identically match the runtime lib version otherwise undefined behaviour ensues? Thanks.

[iceboy233]

Yes.

[aardappel]

Yes, the two must match, though the fact that in this particular case it causes silent garbage decoding is pretty bad. Usually when versions mismatch at least a compile error should occur, I guess we didn't think of that when we made the change.

@incubus370 who introduced this in #5210

[alexnixon]

Thanks for the responses guys.

I just hit this issue again in a slightly different context - transitively depending on multiple versions of flatbuffers that differ only by minor version. And talking to another team here at my company, they've also hit this issue via their Apache arrow dependency.

My personal workaround will be to roll back so all my flatbuffer dependency versions match, but I would also be strongly in favor of rolling back the patch that caused this incompatibility as I think it's quite serious breakage.

[aardappel]

We can roll back that particular PR (and re-apply it in a way that generates with a compile error if mismatched), but that will still make 1.11.0 have this issue with the versions before and after it.

We aren't going to do a 1.12.0 for quite a while, though I suppose we can do a 1.11.1 just for Maven?

My bigger thing is that people using mixed versions is apparently more common than I thought, so maybe we should put some more permanent protection in place, like something in the generated code accessing a constant by a name that has the version number baked into it, like flatbuffers.version_1_11 or whatever, that will force a compile error. Does Java ever optimize out constants in other class files? Or is a static function safer?

[alexnixon]

I think I see three issues:

1. what, if anything, do we do about 1.11.0?
2. how do we make incompatibilities fail loudly rather than quitely?
3. what approach do we take to incompatibility issues in the long run?

For 1, thinking more about this, I can't see any great options. A 1.11.1 release with the incompatibility reverted would fix the issue for anyone who hasn't upgraded to 1.11.x yet, but introduces an incompatibility between 1.11.0 and 1.11.1, which is arguably worse.

For 2, I think a runtime version number check between incompatible versions would be great. But I wouldn't want it to fail spuriously (e.g. if 1.10.0 and 1.11.0 are compatible, then it'd be nice not to break). I don't know the answer to your question constant optimization question, but perhaps a static initializer in the generated code which checks a constant in the runtime lib would be enough?

For 3, I think the main options are break backwards compatibility between minor versions, or don't. I don't have much of a feel for how much FB development would be slowed by introducing compatibility guarantees, but semver does set a precedent here.

Spitballing: if it's decided that things will break, then as a user I'd probably end up shading the runtime JAR everywhere and hoping the pain that that introduces is less than that caused by breakage. But in that case perhaps it'd be an option to have flatc also output the sources of the runtime lib, with an arg to set the package name, which would provide shading automatically. But then the performance would become slightly more complicated owing to the multiple Table classes (for example) which would need to be independently JIT'd.

Would be interested to hear your thoughts on the above.

[vglavnyy]

I thinking, source of this issue is concentration on the C++ part of the library.
The absence of C++ ABI in the standard allows ignoring breaking changes. It is impossible to rebuild a C++ solution without the source code of base classes.

1. It is strange to see manual initialization of Table internal state.
   Fields bb_pos...vtable_size should be private.
   In fact, the __init of Table is ctor. Derived classes should delegate initialization to parent __init.
   Changing the fields from protected to private will block incompatible runtime upgrade.
2. Why generated __init and __assign are public?

[aardappel]

@alexnixon I don't want to get into the business of figuring out which versions are "compatible". By design, runtime and generated code should match exactly in version.

@vglavnyy Yes, this is a bit of a mess. The Java implementation was created after the whole library was already designed with C++ in mind, and then evolved over time, so there is plenty that doesn't make sense. As for public/private, we have this same problem in C++ that some things need to be public because the generated code needs access to it, even though it is really intended to be private to the user.

[aardappel]

So I just made a commit to enforce version equality at compile time for Java and C#: c978b9e

This will protect everyone in the future from these kinds of changes causing problems. This will require stricter versioning by users, but any users that were not updating runtime and generated code at the same time were already running a risk, so breaking them will actually make things safer for them.

If we agree this is a good change I will push 1.11.1 to Maven, meaning people can start using that instead of 1.11.0 and avoid these issues. 1.11.1 generated code with any older runtime version will break, and but using older generated code with newer runtime will still be problematic.

[aardappel]

Actually, I could change an internal API (like __assign) to a different name to force older generated code to also break. WDYT?

[aardappel]

Ok, broke an internal API on purpose to make 100% sure no old generated code will cause this problem anymore: b652fcc

[aardappel]

Ok, uploading a new release to Maven, since I won't be able to do this later. Hopefully this will avoid people running into the above problem as much as possible.

[alexnixon]

Sounds great, thanks for the quick action here.

On not wanting to get into the game of assessing compatibility, that's fair enough. I just wanted to highlight that I found it quite surprising - I can depend on two libraries, A and B, which unbeknownst to me use FB internally. Then A bumps the minor version of their FB dependency, and now my app is broken.

I think this choice has implications for best practice of usage of FB, in that without the usual semver semantics, shading the runtime lib really is the only safe way of using FB in a library. Would you agree?

[aardappel]

That's generally a problem, right? Does Maven allow you to have 2 versions of a library in your app somehow?

There's no problem having two copies of FlatBuffers runtime in your app (if this is possible in Java).. but each copy needs to be accessed with matching generated code.

Knowing what is a breaking change (and thus using semver correctly) is much harder in FlatBuffers than in the average library because of the interplay with generated code. Additionally we support many languages, most of which I don't actually use (including Java), so tracking this becomes impractical. Thus we really don't use semver, every release is a major change that may break the API (and certainly the internal APIs generated code relies on).

As what that does for best practice in Java, I'm not quite sure, but I don't have a better option at this point.