Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An alts release built with JDK 9+ wouldn't work under Java 7 or 8 #7348

Closed
cpovirk opened this issue Aug 21, 2020 · 4 comments
Closed

An alts release built with JDK 9+ wouldn't work under Java 7 or 8 #7348

cpovirk opened this issue Aug 21, 2020 · 4 comments
Assignees
Milestone

Comments

@cpovirk
Copy link
Contributor

cpovirk commented Aug 21, 2020

From #6829 (comment), I gather that your process may be to build releases with JDK 8. If so, then this is more about convenience for anyone else who might do a local build. (Compare #6839, which dealt with similar calls in core.)

My attempt to turn up other such calls shows some in alts:

$ ./gradlew publishToMavenLocal

$ printf '%s\n' ~/.m2/repository/io/grpc/*/1.32.0-SNAPSHOT/*-SNAPSHOT.jar | parallel -q --gnu --will-cite -k bash -c 'jar -tf $1 | grep -oP ".*(?=[.]class$)" | xargs -r javap -private -v -cp $1' - > /tmp/allclasses

$ ( R='Method java/nio/\w+Buffer[.](clear|flip|limit:[(]I|mark|position:[(]I|reset|rewind).*[)]Ljava/nio/\w+Buffer;'; grep -P "$R|^Classfile" /tmp/allclasses | grep -P -B 1 "$R" )
Classfile jar:file:///usr/local/google/home/cpovirk/.m2/repository/io/grpc/grpc-alts/1.32.0-SNAPSHOT/grpc-alts-1.32.0-SNAPSHOT.jar!/io/grpc/alts/internal/AltsTsiHandshaker.class
        66: invokevirtual #163                // Method java/nio/ByteBuffer.limit:(I)Ljava/nio/ByteBuffer;
        84: invokevirtual #168                // Method java/nio/ByteBuffer.position:(I)Ljava/nio/ByteBuffer;
--
Classfile jar:file:///usr/local/google/home/cpovirk/.m2/repository/io/grpc/grpc-alts/1.32.0-SNAPSHOT/grpc-alts-1.32.0-SNAPSHOT.jar!/io/grpc/alts/internal/AltsFraming.class
        38: invokevirtual #59                 // Method java/nio/ByteBuffer.limit:(I)Ljava/nio/ByteBuffer;
        57: invokevirtual #68                 // Method java/nio/ByteBuffer.position:(I)Ljava/nio/ByteBuffer;
        53: invokevirtual #59                 // Method java/nio/ByteBuffer.limit:(I)Ljava/nio/ByteBuffer;
        70: invokevirtual #68                 // Method java/nio/ByteBuffer.position:(I)Ljava/nio/ByteBuffer;
--
Classfile jar:file:///usr/local/google/home/cpovirk/.m2/repository/io/grpc/grpc-alts/1.32.0-SNAPSHOT/grpc-alts-1.32.0-SNAPSHOT.jar!/io/grpc/alts/internal/AltsHandshakerClient.class
        70: invokevirtual #399                // Method java/nio/ByteBuffer.position:(I)Ljava/nio/ByteBuffer;
        83: invokevirtual #399                // Method java/nio/ByteBuffer.position:(I)Ljava/nio/ByteBuffer;
--
Classfile jar:file:///usr/local/google/home/cpovirk/.m2/repository/io/grpc/grpc-alts/1.32.0-SNAPSHOT/grpc-alts-1.32.0-SNAPSHOT.jar!/io/grpc/alts/internal/AltsFraming$Parser.class
        74: invokevirtual #72                 // Method java/nio/ByteBuffer.flip:()Ljava/nio/ByteBuffer;
       184: invokevirtual #118                // Method java/nio/ByteBuffer.limit:(I)Ljava/nio/ByteBuffer;
       210: invokevirtual #72                 // Method java/nio/ByteBuffer.flip:()Ljava/nio/ByteBuffer;
         4: invokevirtual #132                // Method java/nio/ByteBuffer.clear:()Ljava/nio/ByteBuffer;
--
Classfile jar:file:///usr/local/google/home/cpovirk/.m2/repository/io/grpc/grpc-alts/1.32.0-SNAPSHOT/grpc-alts-1.32.0-SNAPSHOT.jar!/io/grpc/alts/internal/AltsFraming$Producer.class
        25: invokevirtual #78                 // Method java/nio/ByteBuffer.flip:()Ljava/nio/ByteBuffer;
        45: invokevirtual #83                 // Method java/nio/ByteBuffer.limit:(I)Ljava/nio/ByteBuffer;
        88: invokevirtual #98                 // Method java/nio/ByteBuffer.position:(I)Ljava/nio/ByteBuffer;
         4: invokevirtual #103                // Method java/nio/ByteBuffer.clear:()Ljava/nio/ByteBuffer;
        16: invokevirtual #98                 // Method java/nio/ByteBuffer.position:(I)Ljava/nio/ByteBuffer;
        36: invokevirtual #83                 // Method java/nio/ByteBuffer.limit:(I)Ljava/nio/ByteBuffer;

Again, there is no problem with 1.31.1, which I assume you built with JDK 8:

$ wget https://repo1.maven.org/maven2/io/grpc/grpc-alts/1.31.1/grpc-alts-1.31.1.jar

$ javap -private -v -cp grpc-alts-1.31.1.jar 'io.grpc.alts.internal.AltsFraming' | grep limit
...
        38: invokevirtual #59                 // Method java/nio/ByteBuffer.limit:(I)Ljava/nio/Buffer;

Still, after a bad protobuf release (protocolbuffers/protobuf#7827) and a near miss with Guava (google/guava#3994), I figured it was worth bringing this up again in case you want to take additional measures.

@cpovirk
Copy link
Contributor Author

cpovirk commented Aug 21, 2020

(Also, if you know of other projects who are likely to be affected (Google-run or otherwise), please spread the word.)

@ejona86
Copy link
Member

ejona86 commented Aug 21, 2020

I'm so very sorry for whatever your day must have been like. Those commands say it all. 🤣 Except then I looked at protocolbuffers/protobuf#7827 and there was more!

Yes, we build with Java 8. When I saw #6839 I wondered why animalsniffer didn't notice; but I also didn't understand our change (not enough context was given; thanks for this wonderful report). I'm not wild about the approach taken in #6839; I'd much rather downcast to Buffer manually at each limit location or use a utility that implicitly does the same, but without an automated checker there's likely regressions no matter the approach.

It seems like we should just give animalsniffer a bit more time to fix this. I've subscribed to the issue and PR so I can upgrade the version when released. We aren't at risk of swapping to Java 11 for compilation of our releases within the next ~year.

I'm strongly tempted to make changes now, but I know there's some PRs in flight that would immediately regress our state. So I'm just going to leave things "as-is" until we have a way to prevent regressions.

CC @voidzcy

@ejona86 ejona86 added this to the 1.32 milestone Aug 21, 2020
@voidzcy
Copy link
Contributor

voidzcy commented Aug 21, 2020

I'd much rather downcast to Buffer manually at each limit location or use a utility that implicitly does the same

Yes, I also noticed the existing awkwardness (take in a ByteBuffer but saved as a Buffer). I was about to change it the other way (only cast to Buffer when necessary), but thought that would be an excessive change for my PR. I can quickly fix it now.

@voidzcy
Copy link
Contributor

voidzcy commented Aug 26, 2020

Existing usages are fixed in #7349. Created #7360 to fix animal sniffer (either we need some configurations or something is missing from animal sniffer side).

@voidzcy voidzcy closed this as completed Aug 26, 2020
@voidzcy voidzcy self-assigned this Feb 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants