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

core: Remove builder class signature rewriter #10406

Merged
merged 2 commits into from Oct 3, 2023

Conversation

sergiitk
Copy link
Member

@sergiitk sergiitk commented Jul 22, 2023

  1. It's been long enough. Removes temporary ABI hack core: Rewrite builder class signatures to avoid internal class #7834.
  2. Removes AbstractManagedChannelImplBuilder and AbstractServerImplBuilder
  3. Introduces ForwardingChannelBuilder2, see details in Tracking Issue for ForwardingChannelBuilder2 being Experimental #10585
  4. Closes Hide AbstractManagedChannelImplBuilder from public API #7211
  5. Fixes javadoc — makes the docs for the inherited methods in the channel and server builders visible:
    NettyChannelBuilder before and after:
    image
    image
    NettyServerBuilder before and after:
    image
    image

ABI BREAKING!

This breaks the ABI of the classes listed below.

Users that recompiled their code using grpc-java v1.36.0 (released on Feb 23, 2021) and later, ARE NOT AFFECTED.

Users that compiled their source using grpc-java earlier than v1.36.0 need to recompile when upgrading to grpc-java v1.59.0. Otherwise the code will fail on runtime with NoSuchMethodError. For example, code:

NettyChannelBuilder.forTarget("localhost:100").maxRetryAttempts(2);

Will fail with

java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractManagedChannelImplBuilder io.grpc.netty.NettyChannelBuilder.maxRetryAttempts(int)'

Server builder example: code

NettyServerBuilder.forPort(80).directExecutor();

Will fail with

java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractServerImplBuilder io.grpc.netty.NettyServerBuilder.directExecutor()'

Affected classes

Class AbstractManagedChannelImplBuilder is deleted, and no longer in the class hierarchy of the channel builders:

  • io.grpc.netty.NettyChannelBuilder
  • io.grpc.okhttp.OkhttpChannelBuilder
  • grpc.cronet.CronetChannelBuilder

Class AbstractServerImplBuilder is deleted, and no longer in the class hierarchy of the server builders:

  • io.grpc.netty.NettyServerBuilder
  • io.grpc.inprocess.InProcessServerBuilder

Related work

Hide AbstractManagedChannelImplBuilder — initial attempt

  1. Issue Hide AbstractManagedChannelImplBuilder from public API #7211
  2. PR Channel builders extend public API #7359
  3. PR Server builders extend public API #7386
  4. PR all: remove deprecated internal OverrideAuthorityChecker #7418
  5. PR core: Inline AbstractManagedChannelImplBuilder #7424
  6. PR core: Inline AbstractServerImplBuilder #7432
  7. Released in v1.33.0

ABI Backward Compatibility Support

  1. Issue Binary backwards-incompatibily in 1.33.0 #7552
  2. PR core: Rewrite builder class signatures to avoid internal class #7834
  3. Issue maxInboundMessageSize is not applied when app code has newer gRPC version than used in library #8313
  4. Fixes sometimes build fails with "Execution failed for task ':grpc-core:compileJava'. > assert i != -1" #8611

This breaks the ABI of the classes listed below.

Users that recompiled  their code using grpc-java [`v1.36.0`]
(https://github.com/grpc/grpc-java/releases/tag/v1.36.0) (released on
Feb 23, 2021) and later, ARE NOT AFFECTED.

Users that compiled their source using grpc-java earlier than
[`v1.36.0`]
(https://github.com/grpc/grpc-java/releases/tag/v1.36.0) need to
recompile when upgrading to grpc-java `v1.59.0`. Otherwise the code
will fail on runtime with `NoSuchMethodError`. For example, code:

```java
NettyServerBuilder.forPort(80).directExecutor();
```

Will fail with

> `java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractServerImplBuilder
  io.grpc.netty.NettyServerBuilder.directExecutor()'`

**Affected classes**

Class `AbstractServerImplBuilder` is deleted, and no longer in the
class hierarchy of the server builders:
- `io.grpc.netty.NettyServerBuilder`
- `io.grpc.inprocess.InProcessServerBuilder`
This breaks the ABI of the classes listed below.

Users that recompiled  their code using grpc-java [`v1.36.0`]
(https://github.com/grpc/grpc-java/releases/tag/v1.36.0) (released on
Feb 23, 2021) and later, ARE NOT AFFECTED.

Users that compiled their source using grpc-java earlier than
[`v1.36.0`]
(https://github.com/grpc/grpc-java/releases/tag/v1.36.0) need to
recompile when upgrading to grpc-java `v1.59.0`. Otherwise the code
will fail on runtime with `NoSuchMethodError`. For example, code:

```java
NettyChannelBuilder.forTarget("localhost:100").maxRetryAttempts(2);
```

Will fail with

> `java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractManagedChannelImplBuilder
  io.grpc.netty.NettyChannelBuilder.maxRetryAttempts(int)'`

**Affected classes**

Class `AbstractManagedChannelImplBuilder` is deleted, and no longer in
the class hierarchy of the channel builders:
- `io.grpc.netty.NettyChannelBuilder`
- `io.grpc.okhttp.OkhttpChannelBuilder`
- `grpc.cronet.CronetChannelBuilder`
@sergiitk
Copy link
Member Author

sergiitk commented Oct 3, 2023

For posterity, here's some tests assets you can use to verify: https://drive.google.com/file/d/1mCHbBbEfz5ku8jEDyUiv6ULfqJ3flhQj/view?usp=share_link

package org.sergii.playground.netty.abi;

import io.grpc.netty.NettyChannelBuilder;

public class Main {

  public static void main(String[] args) {
    NettyChannelBuilder ncb = NettyChannelBuilder.forTarget("localhost:100");

    try {
      // Moved from AbstractManagedChannelImplBuilder to NettyChannelBuilder in 1.33.0
      ncb.maxInboundMessageSize(100);
    } catch (Throwable e) {
      System.out.println(e);
    }

    try {
      // maxRetryAttempts:
      //  <1.33.0: Inherited from AbstractManagedChannelImplBuilder
      //  =1.33.0: Inherited from ForwardingChannelBuilder
      // >=1.33.1: Inherited from tmp AbstractManagedChannelImplBuilder
      // >=1.59.0: Inherited ForwardingChannelBuilder2
      ncb.maxRetryAttempts(200);
    } catch (Throwable e) {
      System.out.println(e);
    }

    // note: maxInboundMetadataSize is the only method AbstractManagedChannelImplBuilder inherits
    //       from ManagedChannelBuilder.

    System.out.println(ncb);
  }
}

<v1.59.x

# v1.32.3 + v1.58.0
❯ java -cp "./builders-abi-1.32.3.jar:./grpc-1.58.0/*" org.sergii.playground.netty.abi.Main
NettyChannelBuilder{delegate=io.grpc.internal.ManagedChannelImplBuilder@1460a8c0}

# v1.35.0 + v1.58.0
❯ java -cp "./builders-abi-1.35.0.jar:./grpc-1.58.0/*" org.sergii.playground.netty.abi.Main
NettyChannelBuilder{delegate=io.grpc.internal.ManagedChannelImplBuilder@1460a8c0}

# v1.36.0 + v1.58.0
❯ java -cp "./builders-abi-1.36.0.jar:./grpc-1.58.0/*" org.sergii.playground.netty.abi.Main
NettyChannelBuilder{delegate=io.grpc.internal.ManagedChannelImplBuilder@1460a8c0}

>=v1.59.x BREAKING

# v1.32.3 + v1.59.x
❯ java -cp "./builders-abi-1.32.3.jar:./grpc-1.59.0-SNAPSHOT/*" org.sergii.playground.netty.abi.Main
java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractManagedChannelImplBuilder io.grpc.netty.NettyChannelBuilder.maxInboundMessageSize(int)'
java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractManagedChannelImplBuilder io.grpc.netty.NettyChannelBuilder.maxRetryAttempts(int)'
NettyChannelBuilder{delegate=io.grpc.internal.ManagedChannelImplBuilder@1460a8c0}

# v1.35.0 + v1.59.x
❯ java -cp "./builders-abi-1.35.0.jar:./grpc-1.59.0-SNAPSHOT/*" org.sergii.playground.netty.abi.Main
java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractManagedChannelImplBuilder io.grpc.netty.NettyChannelBuilder.maxRetryAttempts(int)'
NettyChannelBuilder{delegate=io.grpc.internal.ManagedChannelImplBuilder@1460a8c0}

# v1.36.0 + v1.59.x
❯ java -cp "./builders-abi-1.36.0.jar:./grpc-1.59.0-SNAPSHOT/*" org.sergii.playground.netty.abi.Main
NettyChannelBuilder{delegate=io.grpc.internal.ManagedChannelImplBuilder@1460a8c0}

@sergiitk
Copy link
Member Author

sergiitk commented Oct 3, 2023

cc @njhill

@sergiitk sergiitk merged commit 0e03654 into grpc:master Oct 3, 2023
14 checks passed
@sergiitk sergiitk deleted the undo-magic-magic-methods branch October 3, 2023 17:35
@joroKr21
Copy link

If it's breaking binary backwards compatibility it should be a new major version.
Otherwise we get java.lang.ClassNotFoundException: io.grpc.internal.AbstractManagedChannelImplBuilder

@sergiitk
Copy link
Member Author

sergiitk commented Nov 21, 2023

@joroKr21 Only classes marked with @ExperimentalApi were broken. We could've kept them broken (#7552) three years ago, when the original change breaking ABI was made. But we went out of our way to provide a stop-gap solution (#7834), allowing our users nearly two years to rebuild their code.

@joroKr21
Copy link

joroKr21 commented Nov 21, 2023

Hmm ok, but I don't think I'm using any experimental APIs.
Looking at a stack trace, it seems to be coming from ManagedChannelBuilder.forAddress which is not experimental.

java.lang.NoClassDefFoundError: io/grpc/internal/AbstractManagedChannelImplBuilder
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
	at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862)
	at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at io.grpc.ManagedChannelRegistry.getHardCodedClasses(ManagedChannelRegistry.java:142)
	at io.grpc.ManagedChannelRegistry.getDefaultRegistry(ManagedChannelRegistry.java:103)
	at io.grpc.ManagedChannelProvider.provider(ManagedChannelProvider.java:43)
	at io.grpc.ManagedChannelBuilder.forAddress(ManagedChannelBuilder.java:44)

@ejona86
Copy link
Member

ejona86 commented Nov 21, 2023

@joroKr21, that's probably resolved if you upgrade grpc-netty/grpc-netty-shaded/grpc-okhttp (whichever you are using) to v1.59.0+.

@joroKr21
Copy link

Ok - this is all coming from transitive dependencies, but can I assume that:

  • grpc-netty/grpc-netty-shaded/grpc-okhttp >= v1.59.0 is compatible with grpc-core < v1.59.0
  • grpc-netty/grpc-netty-shaded/grpc-okhttp < v1.59.0 is not compatible with grpc-core >= v1.59.0
  • grpc-netty/grpc-netty-shaded/grpc-okhttp < v1.59.0 is compatible with grpc-core < v1.59.0
  • grpc-netty/grpc-netty-shaded/grpc-okhttp >= v1.59.0 is compatible with grpc-core >= v1.59.0

So the only problem is when core is higher version than netty etc?

@ejona86
Copy link
Member

ejona86 commented Nov 22, 2023

grpc-okhttp uses internal APIs of grpc-core (which is only internal APIs these days). So technically the only thing guaranteed to work is using the same version. grpc-bom can make that easier. Previously we used version pinning (e.g., [1.59.0]) to have Maven complain if there was a skew, but Maven wasn't reliable and Gradle didn't consider that an error, so it was removed in v1.56.0.

Things have happened to be compatible for a while if you do not downgrade dependencies. So grpc-core could be newer than grpc-okhttp and things have worked fine for a while it seems. That's because we are generally adding new things, and not removing stuff.

This specific instance we removed something from grpc-core, so yeah, upgrading it is what caused the problem.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants