Java compile version issue when upgrading to 3.12.4

[kolea2]

What version of protobuf and what language are you using?
Version: protobuf-java 3.12.4
Language: Java

What did you do?
Steps to reproduce the behavior:

When executing this line of code (https://github.com/googleapis/java-bigtable-hbase/blob/master/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/scanner/ReadRowsAcceptanceTest.java#L184), we are seeing this error message:

[ERROR] test[twoRowsEmptyValue](com.google.cloud.bigtable.grpc.scanner.ReadRowsAcceptanceTest)  Time elapsed: 0 s  <<< ERROR!
java.lang.NoSuchMethodError: java.nio.CharBuffer.flip()Ljava/nio/CharBuffer;
	at com.google.protobuf.TextFormat$Parser.toStringBuilder(TextFormat.java:1686)
	at com.google.protobuf.TextFormat$Parser.merge(TextFormat.java:1670)
	at com.google.protobuf.TextFormat$Parser.merge(TextFormat.java:1642)
	at com.google.protobuf.TextFormat.merge(TextFormat.java:1448)
	at com.google.cloud.bigtable.grpc.scanner.ReadRowsAcceptanceTest.addResponses(ReadRowsAcceptanceTest.java:184)
	at com.google.cloud.bigtable.grpc.scanner.ReadRowsAcceptanceTest.test(ReadRowsAcceptanceTest.java:173)

This started when upgrading from protobuf-java 3.12.2 to 3.12.4. I can confirm that testing locally, the test fails with a Java 8 build and passes with Java 11 (CharBuffer.flip() changed between those two versions of java). Were there any changes to the language compile level in protobuf-java recently that would explain this?

Thanks for your help!

[suztomo]

I'm able to reproduce the issue in a simple Maven project https://github.com/suztomo/protobuf-i7827.

I found a similar issue which says "-source 8 -target 8" generates wrong bytecode and ''--release 8" fixes this.

[cpovirk]

It sounds like the linkage checker may soon help detect this, but FYI, another option is to run Animal Sniffer, as we do in Guava.

I've heard mixed things about the --release flag. It may be simplest to just build with JDK8. But as long as Animal Sniffer or an updated linkage checker is in place, I would assume that anything that works is fine.

If you want to avoid this particular error without changing compiler settings, you may be able to change the code to upcast the buffer to Buffer before calling flip(). Maybe I'll put together a fix.

[edit: a post with some explanation]

[cpovirk]

Uh-oh:

• Animal Sniffer detects this, and it logs, but it does not report an error:
  • Signature failure does not halt compilation mojohaus/animal-sniffer#77
  • Remove incorrect handling of covariant return types mojohaus/animal-sniffer#83
  • Treat covariant return type as an error mojohaus/animal-sniffer#87
• The signature changes affect dozens of calls in protobuf. So while we could cast to Buffer before every call (or maybe change the types of some fields/variables/parameters), it would be a pain, and without an automated check, it would likely regress.

--release 8 (or just building with JDK 8, which maven-enforcer-plugin can require) sounds potentially more promising.

(I assume you're building releases with Maven, but I see some (Bazel) BUILD files, too, so it's possible that I'm wrong about that.)

[cpovirk]

Oh, and I had trouble building anything with Maven with JDK11+ because of a maven-surefire-plugin problem with SystemUtils.isJavaVersionAtLeast in recent versions of Java. It's probably fixable by upgrading maven-surefire-plugin and/or some of its libraries.

[edit: The problem is https://issues.apache.org/jira/browse/SUREFIRE-1439, and it's apparently fixable by upgrading to maven-surefire-plugin 2.21.0. I have sent #7830 to update to the newest version, 3.0.0-M5, but it's stuck because CI uses an old version of Maven.]

It's possible that -DskipTests would at least let you get far enough to see the non-fatal Animal Sniffer messages. If not, another hacky workaround (not to be committed) is to stick this in the parent's pluginManagement:

        <plugin>
          <artifactId>maven-surefire-plugin</artifactId>
          <executions>
            <execution>
              <id>default-test</id>
              <phase />
            </execution>
          </executions>
        </plugin>

[cpovirk]

It sounds like the right solution here is to pass --release to javac (by setting <release> for maven-compiler-plugin).

The pom.xml suggests that you're actually targeting Java 7, not 8, so that would be <release>7</release>, replacing the existing <source> and <target>.

The downside to that is that you need to require that developers who build protobuf themselves build with JDK 9+ to use --release. You could hide the <release> setting behind a <profile> so that you can disable it for JDK 8, but that just means that, when you build with JDK 8, you still won't have a strong guarantee of compatibility with Java 7. (That risk might be tolerable, but it also seems easy enough to avoid.)

[cpovirk]

Ah, but --release prevents usage of Unsafe.

I then tried:

<compilerArgs><arg>--add-exports=java.base/jdk.internal.misc=ALL-UNNAMED</arg></compilerArgs>

But:

error: option --add-exports not allowed with target 1.7

(nor with target 1.8, so (a) you can't just bump to that (even if you wanted to) and (b) this doesn't help us for Guava, either)

The only truly correct solution may be to build with Java 7 [edit: but I don't know how well supported that is nowadays, including through Maven] (or at least with Java 8 pointed at a Java 7 bootclasspath). Animal Sniffer can help somewhat, but the fact that it doesn't actually fail the build is unfortunate. Maybe the best bet is a combination of (a) requiring building with Java 8 and (b) asking Animal Sniffer to look for compatibility with Java 7?

[cpovirk]

Err, I may have actually seen some evidence that Animal Sniffer's signatures for Java 7 likewise reject usage of Unsafe? If so, I was hoping that there was a set of signatures for java17-sun, but it appears not. (Oddly, the java18 set of signatures that we use for Guava permits Unsafe.)

So, if you want to use Animal Sniffer, you'll probably have to mark your Unsafe-using code with Animal Sniffer's @IgnoreJRERequirement annotation (or a custom annotation you configure to avoid that dependency).

Supposedly you can generate your own signatures for the JDK or anything else, but that sounds like a pain.

[cpovirk]

FYI, in Guava, we are adding the upcasts (indirectly, by introducing helper methods): google/guava#3994

Hopefully we will do more to actually detect such problems. Fortunately, as I note in internal issue 156345036, it seems likely that the *Buffer classes are the only case that commonly requires special handling:

• https://github.com/bazelbuild/bazel/blob/4c2679f4fd102a35c304ecb2be94ecb9156df91f/src/tools/android/java/com/google/devtools/build/android/desugar/covariantreturn/NioBufferRefConverter.java#L33
• https://bugs.openjdk.java.net/browse/JDK-5096552

We're hoping to not require building with JDK 8:

• It would prevent us from conditionally using JDK 11 APIs like VarHandle.
• It would complicate testing with newer JDKs.