Protobuf: 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.

[cpovirk]

Uh-oh:

• Animal Sniffer detects this, and it _logs_, but it does not report an _error_:
  
  
  
  • https://github.com/mojohaus/animal-sniffer/issues/77
  
  
  • https://github.com/mojohaus/animal-sniffer/pull/83
  
  
  • https://github.com/mojohaus/animal-sniffer/pull/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 https://github.com/protocolbuffers/protobuf/pull/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): https://github.com/google/guava/pull/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 [e-mail]
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
https://github.com/protocolbuffers/protobuf/issues/7827#issuecomment-680910295,
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]

I see 2 files that use `Unsafe:

• https://github.com/protocolbuffers/protobuf/blob/70b02861f8e8ba711efd187188dfb930db7bcaba/java/core/src/main/java/com/google/protobuf/MessageSchema.java#L102
• https://github.com/protocolbuffers/protobuf/blob/70b02861f8e8ba711efd187188dfb930db7bcaba/java/core/src/main/java/com/google/protobuf/UnsafeUtil.java#L45

If you want maximum performance, there is no replacement for them when targeting Java 7 or Java 8 :(

(Under Java 9+, you could probably use VarHandle for at least some of your usages. I don't know if if covers them all. If not, Java has gradually added more replacements for Unsafe APIs.)

[TeBoring]

Then, for now, seems either do the casting for Buffer or adding Unsafe to
classpath (assuming our change could cover our user, please confirm)

On Wed, Aug 26, 2020 at 10:06 AM Chris Povirk [e-mail]
wrote:

>
>

I see 2 files that use `Unsafe:

-
https://github.com/protocolbuffers/protobuf/blob/70b02861f8e8ba711efd187188dfb930db7bcaba/java/core/src/main/java/com/google/protobuf/MessageSchema.java#L102

-
https://github.com/protocolbuffers/protobuf/blob/70b02861f8e8ba711efd187188dfb930db7bcaba/java/core/src/main/java/com/google/protobuf/UnsafeUtil.java#L45

If you want maximum performance, there is no replacement for them when
targeting Java 7 or Java 8 :(

(Under Java 9+, you could probably use VarHandle
https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/VarHandle.html
for at least some of your usages. I don't know if if covers them all. If
not, Java has gradually added more replacements for Unsafe APIs.)

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/protocolbuffers/protobuf/issues/7827#issuecomment-681007737,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABHUPZOAGKOXW5WMOWDZRIDSCU6JTANCNFSM4QDS7ORA
.

[cpovirk]

I believe that either of (a) casting to Buffer or (b) building with --release 7 and Unsafe on the classpath is enough to solve the problem for users, yes.

And it looks like you can indeed manually put Unsafe on the classpath when you use --release 7. There's a copy in $JAVA_HOME/lib/ct.sym under /9A/sun/misc/Unsafe.sig. We'd need to:

• unzip/unjar that somewhere
• move+rename it to have path sun/misc/Unsafe.class
• add that to the classpath

[edit: a previous discussion of this alternative\, and a feature request about more directly supporting Unsafe with --release]

[kolea2]

Thank you all for the detailed analysis on the issue. I think casting to Buffer would solve the issue at hand as well, though I'd worry there are similar issues that we have not yet come across. I'd also like to understand what changed between 3.12.2 and 3.12.4 that initially broke this?

[cpovirk]

Probably 3.12.2 was built with JDK 8 but 3.12.4 was built with JDK 11.

The good news is that Buffer might be the only common API affected.

[TeBoring]

@cpovirk you mean our maven released jar is built with jdk 11 for 3.12.4, but was jdk 8 for 3.12.2?

[cpovirk]

Right. The default JDK installed on Google workstations changed from 8 to 11 on June 23.

[TeBoring]

Is there any internal flag we can switch to jdk8?

[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 <
[email protected]> 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
https://github.com/protocolbuffers/protobuf/issues/7827#issuecomment-681089225,
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.