Skip to content

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Oct 5, 2020

Resolves #26552

Description

In Java 11, ByteBuffer.position(int) is overridden to return a ByteBuffer rather than a Buffer, which causes errors on Android that look like:

java.lang.NoSuchMethodError: No virtual method position(I)Ljava/nio/ByteBuffer; in class Ljava/nio/ByteBuffer; or its super classes (declaration of 'java.nio.ByteBuffer' appears in /system/framework/core-oj.jar)

The workaround is to cast to Buffer before calling position(int) so that we get the implementation from the superclass Buffer, which returns a Buffer as it does in Java 7/8. Thanks to @aaronoe for reporting this. I sent him a local build with this change, and he confirmed that it works on Android 9 & below.

Customer Impact

Allows the Your Phone team to continue working to adopt the Java SignalR client.

Regression?

No, this bug existed in our initial implementation of the MessagePack protocol.

Risk

Low, our test suite still passes and the Your Phone team confirmed that the fix worked for them. Behavior should remain as intended.

@wtgodbe wtgodbe added this to the 5.0.0 milestone Oct 5, 2020
@ghost ghost added the area-signalr Includes: SignalR clients and servers label Oct 5, 2020
@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 5, 2020

/Users/runner/work/1/s/.dotnet/sdk/5.0.100-rc.1.20452.10/NuGet.targets(130,5): error : Response status code does not indicate success: 503 (Service Unavailable - TF10216: Azure DevOps services are currently unavailable. Try again later. Activity Id: 6551d113-b41e-4847-8c06-178e301fae2d (DevOps Activity ID: 6551D113-B41E-4847-8C06-178E301FAE2D)).

Unrelated restore error - will retry in a bit.

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to fill out the template?

@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 5, 2020

Do we need to fill out the template?

Yeah, gonna do that & send email once CI is green

@halter73
Copy link
Member

halter73 commented Oct 5, 2020

In Java 11, ByteBuffer.position(int) is overridden to return a ByteBuffer rather than a Buffer, which causes errors on Android that look like:

java.lang.NoSuchMethodError: No virtual method position(I)Ljava/nio/ByteBuffer; in class Ljava/nio/ByteBuffer; or its super classes (declaration of 'java.nio.ByteBuffer' appears in /system/framework/core-oj.jar)

How does this fix actually work? Have we tested it on Android 9 or earlier?

ByteBuffer is a Buffer, so if the position doesn't exist on the ByteBuffer object, it still wouldn't exist on ByteBuffer object after it was cast to a Buffer unless there's some implicit conversion going on that I'm unaware of. AFAIK Java doesn't have the equivalent of C#'s new keyword in method signatures to do a non-virtual override of sorts.

ByteBuffer.position() isn't supported on Android 9 or earlier, we should find an alternative.

Not exactly. Looking at how msgpack/msgpack-java#516 was resolved, it looks like they fixed it by compiling against an older version of Java. It seems that the real breaking change was Buffer.position(int) switching from final to virtual between Java 8 and Java 9, so it makes sense that compiling using an older Java would fix that.

I'm not sure how casting to Buffer would fix that since the runtime would still be looking for a virtual Buffer.position(int) and "No virtual method position(I)" (emphasis mine) would exist either before or after the cast to Buffer on Android 9 because Buffer.position(int) is final on older runtimes.

@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 5, 2020

It's not that position doesn't exist on the object, it's that the byte code at execution time is unexpected - I got the workaround from here: https://jira.mongodb.org/browse/JAVA-2559. @aaronoe tested this fix out and confirmed that it works on Android 9 & earlier, whereas what we currently have doesn't

@halter73
Copy link
Member

halter73 commented Oct 5, 2020

Java 9 introduces overridden methods with covariant return types

👍 Now it makes sense to me.

@pranavkm pranavkm added the Servicing-consider Shiproom approval is required for the issue label Oct 5, 2020
@ghost
Copy link

ghost commented Oct 5, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 6, 2020
@wtgodbe wtgodbe merged commit d5b5f08 into release/5.0 Oct 6, 2020
@wtgodbe wtgodbe deleted the wtgodbe/ByteBiff branch October 6, 2020 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants