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

animal sniffer does not catch ByteBuffer's usages that break in Java 8 when compiled with Java 9+ #7360

Open
voidzcy opened this issue Aug 26, 2020 · 6 comments
Milestone

Comments

@voidzcy
Copy link
Contributor

voidzcy commented Aug 26, 2020

Originally reported in #7348. Existing usages are manually fixed by #7349. But we'd want to prevent this happens in the future.

Java 9 introduces overridden methods with covariant return types for the following methods in java.nio.ByteBuffer:

  • position​(int newPosition)
  • limit​(int newLimit)
  • flip​()
  • clear​()
  • mark​()
  • reset​()
  • rewind​()

In Java 9 they all now return ByteBuffer, whereas the methods they override return Buffer, resulting in exceptions like this when executing on Java 8 and lower:

java.lang.NoSuchMethodError: java.nio.ByteBuffer.limit(I)Ljava/nio/ByteBuffer

This is because the generated byte code includes the static return type of the method, which is not found on Java 8 and lower because the overloaded methods with covariant return types don't exist (the issue appears even with source and target 8 or lower in compilation parameters). The solution is to cast ByteBuffer instances to Buffer before calling the method.

We'd expect animal sniffer to catch usages that could potentially break Java 8 users.

@ejona86
Copy link
Member

ejona86 commented Aug 26, 2020

#7348 references protocolbuffers/protobuf#7827 (comment) which references mojohaus/animal-sniffer#77 . I was just going to wait for a fix, checking each release, or noticing an update notification.

@ogolberg
Copy link

ogolberg commented Aug 26, 2020

note that my PR (mojohaus/animal-sniffer#83) addressing this issue in animalsniffer has been open for a while - doesn't look like there's enough interest upstream to get it merged

at Toast, we're using a private fork of animalsniffer with the above fix applied

@cpovirk
Copy link
Contributor

cpovirk commented Aug 26, 2020

Yeah, based on your experience so far, I am likewise not too optimistic. I am hoping that maybe, if I post a beginning-to-end repro of the runtime failure, that will get some more traction on the PR. But we'll see.

@cpovirk
Copy link
Contributor

cpovirk commented Aug 26, 2020

(I should probably also mention why --release, their recommended alternative, isn't a great solution in our case.)

@ejona86
Copy link
Member

ejona86 commented Aug 26, 2020

@ogolberg, yeah, I had noticed it had been going on for a while. Although I had also seen the activity in July and figured in 1-2 more months I'd get more pessimistic, but I guess the useful discussion was with other users and not maintainers.

@cpovirk
Copy link
Contributor

cpovirk commented Aug 26, 2020

I gave it a shot in mojohaus/animal-sniffer#77 (comment)

@ejona86 ejona86 modified the milestones: 1.32, 1.33 Aug 27, 2020
@dapengzhang0 dapengzhang0 modified the milestones: 1.33, 1.34 Oct 6, 2020
@voidzcy voidzcy modified the milestones: 1.34, 1.35 Nov 18, 2020
@ericgribkoff ericgribkoff modified the milestones: 1.35, 1.36 Jan 7, 2021
@ejona86 ejona86 modified the milestones: 1.36, Next Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants