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

NoSuchMethodException due to OkHttpChannelProvider default constructor missing after R8 full mode optimization #9611

Open
bubenheimer opened this issue Oct 10, 2022 · 15 comments
Assignees
Milestone

Comments

@bubenheimer
Copy link

bubenheimer commented Oct 10, 2022

What version of gRPC-Java are you using?

1.49.2

What is your environment?

Android

What did you see?

When run after R8 full mode optimization, grpc throws an exception during execution:

Failed to construct OkHttpChannelProvider

java.lang.NoSuchMethodException: io.grpc.okhttp.OkHttpChannelProvider.<init> []
at java.lang.Class.getConstructor0(Class.java:2363)
at java.lang.Class.getConstructor(Class.java:1759)
at io.grpc.android.AndroidChannelBuilder.<clinit>(SourceFile:14)
at com.example.GrpcServiceModule$channel$1.invokeSuspend(SourceFile:51)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(SourceFile:6)
at kotlinx.coroutines.DispatchedTask.run(SourceFile:108)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(SourceFile:77)

This is because R8 full mode does not implicitly keep default constructors:
https://r8.googlesource.com/r8/+/refs/heads/master/compatibility-faq.md#r8-full-mode

This Proguard/R8 configuration rule avoids the issue and should be added to https://github.com/grpc/grpc-java/blob/master/android/proguard-rules.txt:

-keepclassmembers, allowoptimization class io.grpc.okhttp.OkHttpChannelProvider {
    <init>();
}

There may be additional configuration rules needed to take care of further default constructors accessed via reflection.

@bubenheimer bubenheimer changed the title throws Exception NoSuchMethodException due to OkHttpChannelProvider default constructor missing from R8 full mode Oct 10, 2022
@bubenheimer bubenheimer changed the title NoSuchMethodException due to OkHttpChannelProvider default constructor missing from R8 full mode NoSuchMethodException due to OkHttpChannelProvider default constructor missing after R8 full mode optimization Oct 10, 2022
@ejona86
Copy link
Member

ejona86 commented Oct 10, 2022

So it keeps the class and methods (because the interface methods are used), but it doesn't have any way to construct the class.

We've taken the approach of "don't use any Proguard configuration" because it is hard to get used in all environments and have a configuration that is precise for all environments. We want to inform the optimizer via code that stuff is used, because that code may be eliminated.

But this behavior from R8 should also break using ManagedChannelBuilder.forTarget() and similar APIs. We'd need to keep the init for all NameResolvers, LoadBalancers, and ManagedChannelProviders. But apparently you didn't... So I guess it saw the META-INF/services/ and recognized those classes needed default constructors? More investigation is needed here.

@bubenheimer
Copy link
Author

The proguard-rules.txt file that I mentioned is incorporated in the grpc-android artifact, providing Proguard configuration for grpc-android library consumers.

A good amount of grpc (OkHttp Android client) communication seems to work for me without further R8/Proguard configuration than what's referenced in this issue, I don't currently use LoadBalancer stuff AFAIK. I did encounter some intermittent issues in my cursory exploration runs today, but they may or may not be related. I'm not sure if I'll find more time to investigate before next month.

Android has default Proguard/R8 configuration files that are commonly used and that I rely on, and there is additional Proguard configuration coming from other libraries I use, and some of my own. Any of these could lead to an unexpected smooth operation of those things.

Slapping an explicit @Keep on needed constructors would likely be another way to retain them, but I don't know if it has to come from androidx.annotation (or android.annotation) or if R8 understands others, like the one from errorprone.

@bubenheimer
Copy link
Author

bubenheimer commented Oct 14, 2022

I've given up on R8 full mode for now; grpc things seemed to work well for me, though, with that one added rule. I got hung up on other hard-to-debug problems. R8 full mode seemed to work for me in the past here and there, but it seems to have moved beyond real world usability. Or the other way round.

@ejona86
Copy link
Member

ejona86 commented Dec 20, 2022

Seems we won't be doing anything here for the moment. @Keep could work, but is wrong, because we really want the consumer of the API to dictate what needs to be kept, so dead-code elimination can clean up more things when possible. Since it sounds like R8 full mode in not for the feint of heart, we'll delay until someone is sticking with R8 full mode. Maybe it gets easier to work with between then and now.

Things have changed since we did grpc-android initially; we have a much better understanding of how Cronet fits into things. It might be better to add a direct dependency from grpc-android to grpc-okhttp and reference OkHttpChannelProvider directly instead of reflection. But that's this one case; there are probably other problems that would also need resolving (e.g., all the provider registries).

@ejona86 ejona86 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2022
@bubenheimer
Copy link
Author

@ejona86 Agreed. What you say about avoiding reflection with OkHttpChannelProvider sounds like it would make sense independent of this issue. Reflection is still unusually expensive on Android even today I believe.

@bubenheimer
Copy link
Author

This will likely need revisiting soon, as R8 full mode is becoming the default: https://youtu.be/WZ1A7aoEHSw?t=554

@ejona86 ejona86 reopened this Jan 9, 2023
@ejona86
Copy link
Member

ejona86 commented Jan 9, 2023

Well, I slept better for 3 weeks.

@temawi
Copy link
Contributor

temawi commented Jan 25, 2023

@bubenheimer, I tried to reproduce what you are seeing using the gRPC android-interop-testing project by creating a gradle.properties file with the line android.enableR8.fullMode=true.

I do see WARNING:: The option setting 'android.enableR8.fullMode=true' is experimental., so I believe R8 full mode is getting enabled. But I'm able to run the interop test client fine without the NoSuchMethodException.

Would you happen to have a project I could use to reproduce the problem?

@bubenheimer
Copy link
Author

bubenheimer commented Jan 31, 2023

@temawi what's your R8 version? I print it with my my builds like this:

println("D8/R8 version: " + com.android.tools.r8.Version.getVersionString())

Reason I'm asking: not sure that current R8 still prints that message in full mode, it's not experimental anymore. I didn't always have this problem in R8 full mode, so you may not see it with an older R8 version. The R8 from Android Studio Giraffe alpha 2 is 8.1.8-dev. The R8 from the latest stable Android Studio (Electric Eel) (AGP 7.4.0) is 4.0.48, I was using nothing more recent than that, so it should suffice for your repro.

The client likely needs to be Android, and use OkHttp.

I don't have a public project of my own to repro this.

@ejona86
Copy link
Member

ejona86 commented Feb 1, 2023

I expect the R8/toolchain version was just too old. We have been needing to update for a while now.

android-interop-testing doesn't use AndroidChannelBuilder. But it does load (indirectly) OkHttpChannelBuilder with reflection. And both use the same reflection methods (forClass().asSubclass().getConstructor().newInstance()).

@temawi
Copy link
Contributor

temawi commented Feb 1, 2023

Yes, I also understood that that android-interop-testing would use OkHttpChannelBuilder. But my initial test ended up not using R8 as the debug build I was testing with did not have it enabled. It failed after adding minifyEnabled, but not with the error from this issue. I also determined that version 2.2.64 of R8 is being used, which is quite old. I'll work on updating the project and reproducing the problem with OkHttpChannelBuilder.

@ejona86 ejona86 added this to the Next milestone Feb 3, 2023
@temawi
Copy link
Contributor

temawi commented Mar 8, 2023

In my attempt to reproduce this I upgraded all of grpc-java Android projects to use the 7.4.0 version of the Android Gradle plugin. This introduces a newer R8 that does full mode by default as well as moves us to building with Java 11. I did now see some extra stuff being removed by R8 in android-interop-testing that I had to instruct R8 to keep, but I still did not see the original problem with OkHttpChannelProvider.<init>.

@bubenheimer, since I don't feel there's anything else I can do on this front I'll go ahead and close this issue. Feel free to open it back up if you still have a problem after these changes and if you can provide a way to reproduce the problem.

@temawi temawi closed this as completed Mar 8, 2023
@temawi
Copy link
Contributor

temawi commented Mar 8, 2023

One clarification - 7.4.0 does NOT enable R8 full mode by default. I tested this by adding android.enableR8.fullMode=true to my local gradle.properties.

@bubenheimer
Copy link
Author

Thank you, @temawi. Just to clarify: you explicitly set android.enableR8.fullMode=true and did not see the OkHttpChannelProvider issue?

@bubenheimer
Copy link
Author

I was able to repro it with the helloWorld Android client (AGP 7.4.2). I had to add the grpc-android dependency, and use AndroidChannelBuilder instead of ManagedChannelBuilder, that's the key.

Also, for R8 config, add "-shrinkunusedprotofields" to proguard-rules.pro to have R8 do the right magic for protos.

Enable full mode, of course.

You may need to rework that project a little to get it to run in a current Android Studio environment, I think some of the project config is too old. That would be very helpful for Android users anyway.

Also, I noticed that building android-interop-testing seems to require some secret config that is not checked in. At a minimum, android.useAndroidX=true is required in gradle.properties. I had to make a few more changes to get it to work in my environment, not sure what of that was only due to my local config. This was on main branch, and I had the recent Android project config changes on that branch.

@ejona86 ejona86 reopened this Mar 20, 2023
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

3 participants