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

Circular dependency in grpc-core (Gradle) #10701

Closed
EpariNigam opened this issue Nov 23, 2023 · 10 comments
Closed

Circular dependency in grpc-core (Gradle) #10701

EpariNigam opened this issue Nov 23, 2023 · 10 comments
Milestone

Comments

@EpariNigam
Copy link

EpariNigam commented Nov 23, 2023

What version of gRPC-Java are you using?

1.59.0

What is your environment?

Android

What did you expect to see?

There shouldn't be circular dependencies in libraries. We are using google-oss-licenseplugin in our project to show the licenses of libraries we want to use. But, due to this the project is failing to build release version.

What did you see instead?

Seeing circular dependencies in grpc-core -> grpc-util -> grpc-core

+--- io.grpc:grpc-core:1.59.0
|    +--- io.grpc:grpc-api:1.59.0
|    |    +--- com.google.code.findbugs:jsr305:3.0.2
|    |    +--- com.google.errorprone:error_prone_annotations:2.20.0
|    |    \--- com.google.guava:guava:32.0.1-android
|    |         +--- com.google.guava:failureaccess:1.0.1
|    |         +--- com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
|    |         +--- com.google.code.findbugs:jsr305:3.0.2
|    |         +--- org.checkerframework:checker-qual:3.33.0
|    |         +--- com.google.errorprone:error_prone_annotations:2.18.0 -> 2.20.0
|    |         \--- com.google.j2objc:j2objc-annotations:2.8
|    +--- com.google.code.gson:gson:2.10.1
|    +--- com.google.android:annotations:4.1.1.4
|    +--- org.codehaus.mojo:animal-sniffer-annotations:1.23
|    +--- com.google.errorprone:error_prone_annotations:2.20.0
|    +--- com.google.guava:guava:32.0.1-android (*)
|    +--- io.perfmark:perfmark-api:0.26.0
|    +--- io.grpc:grpc-context:1.59.0
|    |    \--- io.grpc:grpc-api:1.59.0 (*)
|    \--- io.grpc:grpc-util:1.59.0
|         +--- io.grpc:grpc-core:1.59.0 (*)
|         +--- org.codehaus.mojo:animal-sniffer-annotations:1.23
|         \--- com.google.guava:guava:32.0.1-android (*)

Steps to reproduce the bug

Adding grpc-core to any library and generating dependency graph.

@ejona86
Copy link
Member

ejona86 commented Nov 25, 2023

This is essentially a duplicate of #10682 #10576. Although that one is more for Bazel. This isn't a "true" circular dependency for most build systems, as it isn't a compile dependency. Looks like there's an existing issue open for that project google/play-services-plugins#239

In truth, it isn't even a real dependency from grpc-core to grpc-util. We split grpc-util out of grpc-core and have the dependency there since users could be surprised at runtime when the round-robin LB (in grpc-util) is not present. And it'd be hard for people to debug.

@ejona86
Copy link
Member

ejona86 commented Nov 25, 2023

That plugin may be suffering from gradle/gradle#22850 , which was mentioned in #10576.

@pkoenig10
Copy link
Contributor

pkoenig10 commented Nov 27, 2023

To add some color to my comment in #10576 (comment), we're currently prevented from upgrading gRPC due to gradle/gradle#22850 and this cyclical dependency.

This is especially problematic because it also is preventing us from upgrading Netty, because upgrading Netty to 4.1.101.Final requires a version of gRPC with #10663. It's only a matter of time before another CVE is discovered in Netty and we are forced to upgrade again, which would put us in a really difficult position.

If this dependency isn't actually necessary, then I would implore you to consider removing this dependency so consumers can continue to upgrade to the latest version of gRPC.

@ejona86
Copy link
Member

ejona86 commented Nov 27, 2023

Can someone provide a reproduction example?

we're currently prevented from upgrading gRPC due to gradle/gradle#22850 and this cyclical dependency.

You're not able to do something like the below?

dependencies {
  implementation ('io.grpc:grpc-netty:1.59.0') {
    exclude group: 'io.grpc', module: 'grpc-util'
  }
}

@ejona86 ejona86 changed the title Circular dependency in grpc-core Circular dependency in grpc-core (Gradle) Nov 28, 2023
@pkoenig10
Copy link
Contributor

pkoenig10 commented Nov 28, 2023

This doesn't work because grpc-netty doesn't have a direct dependency on grpc-core.

The dependencies here look like:

grpc-netty -> grpc-core -> grpc-util -> grpc-core

You can workaround this by:

  • Adding an explicit dependency on grpc-core
  • Excluding grpc-util from grpc-core (to remove the bad dependency)
  • Excluding grpc-core from grpc-netty (so you don't get the bad dependency through the grpc-core dependency in grpc-netty)
dependencies {
  implementation ('io.grpc:grpc-netty') {
    exclude group: 'io.grpc', module: 'grpc-core'
  }
  implementation ('io.grpc:grpc-core') {
    exclude group: 'io.grpc', module: 'grpc-util'
  }
}

And you have to repeat process for any other dependency that transitively brings in grpc-core.

dependencies {
  implementation ('io.grpc:grpc-netty') {
    exclude group: 'io.grpc', module: 'grpc-core'
  }
  implementation ('io.grpc:grpc-core') {
    exclude group: 'io.grpc', module: 'grpc-util'
  }

  // Some other dependency that depends on grpc-netty
  implementation ('com.example:foo') {
    exclude group: 'io.grpc', module: 'grpc-netty'
  }

  // Some other that transitively depends on grpc-netty
  implementation ('com.example:bar') {
    exclude group: 'io.grpc', module: 'grpc-core'
  }
}

This also causes problems in tools that detect unused Gradle dependencies. The grpc-core dependency isn't actually used by this module but we have to add an explicit dependency just so we can exclude grpc-util.

@ejona86
Copy link
Member

ejona86 commented Nov 28, 2023

This doesn't work because grpc-netty doesn't have a direct dependency on grpc-core.

I've definitely excluded transitive dependencies via that approach before.

Did you try it? It seems it works to me.

configurations {
    doesItBreak
}

dependencies {
    doesItBreak('io.grpc:grpc-netty:1.59.1') {
        exclude group: 'io.grpc', module: 'grpc-util'
    }
}

println(configurations.doesItBreak.getResolvedConfiguration().getFirstLevelModuleDependencies().find{ true }.getAllModuleArtifacts())

Without the exclude, I do see the java.lang.StackOverflowError.

@pkoenig10
Copy link
Contributor

Ah sorry, you're right. It didn't seem to be working earlier when I tried, but I must have been doing something wrong,. Trying it again and it does seem to work.

@EpariNigam
Copy link
Author

While this helps, can we avoid this? Even if someone migrates their gradle with the fix from their side. There will be other side effects, if it is a large project.

@ejona86
Copy link
Member

ejona86 commented Dec 4, 2023

I suggest you also post more on gradle/gradle#22850 since changing this in gRPC would be a workaround.

@sergiitk
Copy link
Member

Seems we've done as much as we can for now. Once you have more info or questions, comment, and this can be reopened.

ejona86 added a commit to ejona86/grpc-java that referenced this issue Feb 15, 2024
Add the 'fake' dependency to grpc-netty instead of grpc-core.
grpc-okhttp already depends on grpc-util and probably would be fine
without round_robin on Android.

Worksaround grpc#10857 grpc#10701
ejona86 added a commit to ejona86/grpc-java that referenced this issue Feb 15, 2024
Add the 'fake' dependency to grpc-netty instead of grpc-core.
grpc-okhttp already depends on grpc-util and probably would be fine
without round_robin on Android.

There's not actually a circular dependency, but some tools can't handle
the compile vs runtime distinction. Such tools are broken, but fixes
have been slow and this approach works with no real downfalls.

Worksaround grpc#10857 grpc#10701
ejona86 added a commit to ejona86/grpc-java that referenced this issue Feb 15, 2024
Add the 'fake' dependency to grpc-netty instead of grpc-core.
grpc-okhttp already depends on grpc-util and probably would be fine
without round_robin on Android.

There's not actually a circular dependency, but some tools can't handle
the compile vs runtime distinction. Such tools are broken, but fixes
have been slow and this approach works with no real downfalls.

Works around grpc#10857 grpc#10701
ejona86 added a commit to ejona86/grpc-java that referenced this issue Feb 15, 2024
Add the 'fake' dependency to grpc-netty instead of grpc-core.
grpc-okhttp already depends on grpc-util and probably would be fine
without round_robin on Android.

There's not actually a circular dependency, but some tools can't handle
the compile vs runtime distinction. Such tools are broken, but fixes
have been slow and this approach works with no real downfalls.

Works around grpc#10576 grpc#10701
ejona86 added a commit that referenced this issue Feb 16, 2024
Add the 'fake' dependency to grpc-netty instead of grpc-core.
grpc-okhttp already depends on grpc-util and probably would be fine
without round_robin on Android.

There's not actually a circular dependency, but some tools can't handle
the compile vs runtime distinction. Such tools are broken, but fixes
have been slow and this approach works with no real downfalls.

Works around #10576 #10701
ejona86 added a commit to ejona86/grpc-java that referenced this issue Feb 16, 2024
Add the 'fake' dependency to grpc-netty instead of grpc-core.
grpc-okhttp already depends on grpc-util and probably would be fine
without round_robin on Android.

There's not actually a circular dependency, but some tools can't handle
the compile vs runtime distinction. Such tools are broken, but fixes
have been slow and this approach works with no real downfalls.

Works around grpc#10576 grpc#10701
@ejona86 ejona86 added this to the 1.63 milestone Feb 16, 2024
ejona86 added a commit that referenced this issue Feb 16, 2024
Add the 'fake' dependency to grpc-netty instead of grpc-core.
grpc-okhttp already depends on grpc-util and probably would be fine
without round_robin on Android.

There's not actually a circular dependency, but some tools can't handle
the compile vs runtime distinction. Such tools are broken, but fixes
have been slow and this approach works with no real downfalls.

Works around #10576 #10701
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants