grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.48k stars 3.85k forks source link

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

Open bubenheimer opened 2 years ago

bubenheimer commented 2 years ago

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.

ejona86 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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).

bubenheimer commented 1 year ago

@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 commented 1 year ago

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

ejona86 commented 1 year ago

Well, I slept better for 3 weeks.

temawi commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

temawi commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

bubenheimer commented 1 year ago

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.

crysis74 commented 6 months ago

I have the same problem. When switching version AGP from 8.1.3 to 8.4.0, i get this error. protobuf - 4.27.0-RC2 grpc - 1.63.0 Android Caused by: java.lang.UnsupportedOperationException: Unable to load OkHttpChannelProvider at zc.a.<init>(Unknown Source:7) The proposed rule fixed the problem.

eugene-krivobokov commented 5 months ago

com.android.tools.r8.Version.getVersionString()

It's safer to check the actual R8 version in artifacts. It can use a different version if you override R8 version and run R8 in a separate process: https://issuetracker.google.com/issues/283632726#comment2

ArturoSalazarB16 commented 1 month ago

Hey! is there any fix/workaround for this problem?

My team just migrated our apps to AGP 8.5 and ran into this problem (I'm assuming that Full R8 mode being enabled by default was the trigger)

java.util.ServiceConfigurationError: Provider io.grpc.okhttp.OkHttpChannelProvider could not be instantiated java.lang.NoSuchMethodException: io.grpc.okhttp.OkHttpChannelProvider .<init> []
    at io.grpc.ServiceProviders.createForHardCoded(ServiceProviders.java:147)
    at io.grpc.ServiceProviders.getCandidatesViaHardCoded(ServiceProviders.java:125)
    at io.grpc.ServiceProviders.loadAll(ServiceProviders.java:62)
    at io.grpc.ManagedChannelRegistry.getDefaultRegistry(ManagedChannelRegistry.java:101)
    at io.grpc.ManagedChannelProvider.provider(ManagedChannelProvider.java:43)
    at io.grpc.ManagedChannelBuilder.forTarget(ManagedChannelBuilder.java:86)
        ...
Caused by: java.lang.NoSuchMethodException: io.grpc.okhttp.OkHttpChannelProvider.<init> []                                                                                                   
   at java.lang.Class.getConstructor0(Class.java:2332)                                                                                                   
   at java.lang.Class.getConstructor(Class.java:1728)
ejona86 commented 1 month ago

Workaround would be to disable full mode or to add -keeps.

ArturoSalazarB16 commented 1 month ago

Hi @ejona86 - do you have a defined list of -keeps we could add? (e.g., I was just wondering if there was anything we need to preserve besides theOkHttpChannelProvider constructor)

ejona86 commented 1 month ago

No, I don't have a list. It depends on what you depend on. You probably will need one for io.grpc.internal.DnsNameResolverProvider's and io.grpc.internal.PickFirstLoadBalancer's constructors.

I don't know if there's a way to encode "keep this method if the class is kept". That's what really needed here to avoid full mode's optimizations increasing the total amount of code.