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.

[TeBoring]

Would dropping support for java7 and using --release 8 fix the issue?

[cpovirk]

It would, but --release doesn't let us use Unsafe. I didn't look at how protobuf uses it; I just saw that it used it somewhere.

[TeBoring]

I saw you mentioned cast to Buffer before every call. How many places need to change? Does that also work?

[cpovirk]

Yes, upcasting to Buffer before every call would work. I believe I saw a couple dozen calls, but I don't remember offhand. I'll see if I can look that up quickly.

[cpovirk]

Here's an attempt to identify affected calls (excluding tests): covariants.txt

That's 60 calls -- more than I'd remembered.

[TeBoring]

Just replace all of them seems feasible

…

On Wed, Aug 26, 2020 at 7:20 AM Chris Povirk ***@***.***> wrote: Here's an attempt to identify affected calls (excluding tests): covariants.txt <https://github.com/protocolbuffers/protobuf/files/5130396/covariants.txt> That's 60 calls -- more than I'd remembered. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7827 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHUPZLARRHU2XXZJC3HFLLSCUK3XANCNFSM4QDS7ORA> .

[TeBoring]

BTW, internally we are using java8, why this didn't fail internally?

[cpovirk]

Internally, we build against a Java 8 bootclasspath. It's a lot like building with --release (except that it lets us use Unsafe).

[TeBoring]

How does --release reject Unsafe? A build error? Or only happens with Animal Sniffer?
If so, can we use --release without Animal Sniffer?

[cpovirk]

It produces a javac build error, so it's a problem even without Animal Sniffer.

I have wondered if we could find a workaround (like putting Unsafe on the classpath), but I haven't experimented.

[TeBoring]

Can you point out to me where are those Unsafe?
And what are the suggest replacement for them?

[cpovirk]

You can set $JAVA_HOME, as in:

$ JAVA_HOME=/usr/local/buildtools/java/jdk8 mvn clean install

[elharo]

What's the build and release process for this library? Is it being built on an individual developer workstation with mvn perform:release or equivalent? If so, we might want to switch to kokoro+Rapid for multiple reasons.

[TeBoring]

I think it’s kokoro + docker

…

On Wed, Aug 26, 2020 at 12:51 PM Elliotte Rusty Harold < ***@***.***> wrote: What's the build and release process for this library? Is it being built on an individual developer workstation with mvn perform:release or equivalent? If so, we might want to switch to kokoro+Rapid for multiple reasons. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7827 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHUPZMSKIKIQ4UICQJQMJ3SCVRUNANCNFSM4QDS7ORA> .

[TeBoring]

Sent a fix internally for our release process. Seems we are using the default /usr/local/buildtools/java/jdk.

[elharo]

@TeBoring can you point me to the internal changes you made?

@suztomo we should audit the various release scripts and kokoro configs in google3 to check whether they explicitly specify a JDK. java 8 or Java 11, this should be a deliberate choice, not an accident.

[cpovirk]

@elharo: I just CCed you. (If it's not showing up, search for the URL of this issue.)

Good point about the search. A search like /usr/local/buildtools/java/jdk\b -f:/usr/local/buildtools/java/jdk f:(release|kokoro) turns up at least a couple suspicious files -- one that has the release instructions for libphonenumber and one that sets up the PATH to contain /usr/local/buildtools/java/jdk/bin for probably many release processes. I'm going to raise this on an internal issue, on which I'll also CC you.

[suztomo]

@TeBoring Hi Paul, do you have plan to release a fix to this issue?

[eed3si9n]

I came to report that I ran into a similar issue. Different method from op but one that's listed in convariants.txt so I guess you're aware of it:

Caused by: java.lang.NoSuchMethodError: java.nio.ByteBuffer.position(I)Ljava/nio/ByteBuffer;
  at com.google.protobuf.NioByteString.copyToInternal(NioByteString.java:112)
  at com.google.protobuf.ByteString.toByteArray(ByteString.java:695)
  at com.google.protobuf.NioByteString.writeTo(NioByteString.java:123)
  at org.apache.beam.sdk.extensions.protobuf.ByteStringCoder.encode(ByteStringCoder.java:67)
  at org.apache.beam.sdk.extensions.protobuf.ByteStringCoder.encode(ByteStringCoder.java:37)
  at org.apache.beam.sdk.coders.DelegateCoder.encode(DelegateCoder.java:74)
  at org.apache.beam.sdk.coders.DelegateCoder.encode(DelegateCoder.java:68)

In general, does this prevent people/companies from using protobuf 3.12.4+ if they are on JDK < 10? If I understand correctly, these would sometimes show up as a runtime error, like in this case you could be calling a method on Apache Beam. If so it might be helpful if this issue was hi-lighted better the release note as a known issue.

[jlmuir]

In general, does this prevent people/companies from using protobuf 3.12.4+ if they are on JDK < 10? If I understand correctly, these would sometimes show up as a runtime error, like in this case you could be calling a method on Apache Beam. If so it might be helpful if this issue was hi-lighted better the release note as a known issue.

Not necessarily. It just means that the Protobuf Java developers have to arrange for the protobuf-java library to get built in a way that will make it work on all the supported Java versions. They can do that right now, but they haven't done it yet. Based on the comments above, it could be done in at least three different ways:

1. Force the calls to methods that got covariant return types in Java 9+ to use the return types as they were before Java 9. This is what some people are doing when they talk about casting as a solution and is what the Guava developers did but in a way that does not involve casting (i.e., wrapper methods).
2. Build the protobuf-java library with JDK 7 or 8. Since the protobuf-java library targets Java 7, building with JDK 8 is technically wrong because Java 8 could have introduced covariant returns for some methods just like Java 9 did. But in practice, JDK 8 would work because, based on the comments above, that's how protobuf-java had been built up to and including version 3.12.2 without any ill effects.
3. Compile with the --release 7 flag since apparently, based on the comments above, -source 7 -target 7 does not work correctly (i.e., the compiler miscompiles it). I don't know whether all compilers have this problem or just some. I also don't know whether it's truly a problem with the compiler or actually a problem with the Maven Compiler Plugin. If the Maven Compiler Plugin would work if a JDK 7 boot classpath were specified, then the problem is with how the Protobuf Java developers have configured the build. However, in their defense, it seems lame that the Maven Compiler Plugin would let you configure a target of Java 7 but that it could still result in a miscompile that won't work on Java 7. It seems to me that it shouldn't even allow that; I think if the Maven Compiler Plugin is configured with a target Java version, it should require a JDK boot classpath matching the configured target and fail the build if not provided.

[eed3si9n]

Not necessarily. It just means that the Protobuf Java developers have to arrange for the protobuf-java library to get built in a way that will make it work on all the supported Java versions. They can do that right now, but they haven't done it yet.

I understand that at some point in the future this may be fixed. What I'm saying is that today, if any of the Java/Scala etc libraries pulled in 3.12.4+ (because why not?) it could result to a runtime error in a production system. Thankfully my work code had a decent code coverage, so we caught this, but not everyone might. So I'm asking this issue be highlighted in the release note of 3.12.4, 3.13.0, and 3.13.0.1 as a known regression with regard to JDK 8.

[suztomo]

Beam's protobuf dependency hit this.
https://lists.apache.org/thread.html/r1387b34ec5995c87f079cb41e8f62096f83ed350b8378bd6389416e6%40%3Cdev.beam.apache.org%3E

[iemejia]

Is this fixed in 3.14.0 ?

[suztomo]

I see it's fixed in 3.14.0. Thank you.

[ijuma]

Fyi, we use --release 8 and Unsafe in Kafka and it works. Perhaps the issue you mentioned only happens if the module path is used.

[cpovirk]

Thanks. It looks like the difference is that you never refer directly to the type Unsafe, only through reflection. We are probably avoiding that for performance reasons, but I see that you can use MethodHandle to optimize. (I may have forgotten that MethodHandle is available as far back as... Java 7?)

[ijuma]

Oh, I see. Method handles are available since Java 7, but Android is a bit of a problem.