Flatbuffers: [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.

[iceb0y]

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 https://github.com/google/flatbuffers/pull/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#: https://github.com/google/flatbuffers/commit/c978b9ef1f74c4918a33ee26217c85e5a5949a5b

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: https://github.com/google/flatbuffers/commit/b652fcc3a7708f0cf5027748fd4d2df134eb6974

[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.

[alexnixon]

Without gnarly classloader trickery or shading you can't have two versions in your app, so yes this is a general Java problem, but there are a couple of things which make it more painful:

• frequent breakages
• unexpected breakages (e.g. those introduced in minor or patch versions)

When dependency mismatches occur there are several possible courses of action. Here are some examples from Ivy (also used by SBT), but I don't know what the relative frequency of their usages is.

For reference, the protobuf project seems to handle this by avoiding breakage:

Protobuf minor version releases are backwards-compatible. If your code can build/run against the old version, it's expected to build/run against the new version as well. Both binary compatibility and source compatibility are guaranteed for minor version releases if the user follows the guideline described in this section.

Back to FB, if avoiding incompatibilities is too costly, then my personal conclusion for Java then is:

• If you're going to use flatbuffers in a Java library, then you should either shade it or accept the compatibility risk that you're pushing onto your users.

(if you're not from the Java world - "shading" flatbuffers means, at compile-time, copying the entire contents of FB into your library verbatim, and mangling symbols and references to those symbols in such a way as to make it effectively invisible. So someone depending on you doesn't see you're using FB, and can pull in their own version if they want, without conflicts)

[aardappel]

Protobuf is a way older project and thus has a way stabler codebase. I suspect the same will happen with FlatBuffers where breaking changes happen less and less. They also have (had?) a large centralized team working on it, whereas most changes to FlatBuffers are by external contributors.

Yes, your assessment is thus correct. I would recommend "shading" by default, as FlatBuffers has a really small runtime, and the generated code already needs to be a personal copy anyway. Especially if the library that uses FlatBuffers is itself widely used.

[github-actions[bot]]

This issue is stale because it has been open 6 months with no activity. Please comment or this will be closed in 14 days.