grpc-java: Binary backwards-incompatibily in 1.33.0
My library contains code like:
NettyChannelBuilder ncb = NettyChannelBuilder.forTarget(...);
ncb.setMaxInboundMessageSize(...);
ncb.eventLoopGroup(...);
// ...
ManagedChannel channel = ncb.build();
which breaks when moving from pre-1.33.0 to 1.33.0, unless recompiled (and then the new binary would not be compatible with versions < 1.33.0). This is because a class (AbstractManagedChannelImplBuilder) was removed from the direct superclass heriarchy:
Changing the direct superclass or the set of direct superinterfaces of a class type will not break compatibility with pre-existing binaries, provided that the total set of superclasses or superinterfaces, respectively, of the class type loses no members.
I know that technically netty transport is still considered experimental (per #1784), but (a) it’s the primary transport and (b) it’s been that way for more than 4 years.
Was this intentional/known or is it a case of “it’s experimental, tough!”?
It should be easy to avoid I think by for example just adding an empty abstract class AbstractManagedChannelImplBuilder extends ForwardingChannelBuilder… maybe in a 1.33.1? 😃
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 16 (16 by maintainers)
Commits related to this issue
- xds: Copy ForwardingServerBuilder to xds for XdsServerBuilder This reduces ABI issues caused by returning the more precise XdsServerBuilder in the API. See https://github.com/grpc/grpc-java/issues/75... — committed to ejona86/grpc-java by ejona86 4 years ago
- xds: Copy ForwardingServerBuilder to xds for XdsServerBuilder This reduces ABI issues caused by returning the more precise XdsServerBuilder in the API. See https://github.com/grpc/grpc-java/issues/75... — committed to ejona86/grpc-java by ejona86 4 years ago
- api: Expose ForwardingServerBuilder for XdsServerBuilder This reduces ABI issues caused by returning the more precise XdsServerBuilder in the API. See https://github.com/grpc/grpc-java/issues/7552. — committed to ejona86/grpc-java by ejona86 4 years ago
- api: Expose ForwardingServerBuilder for XdsServerBuilder This reduces ABI issues caused by returning the more precise XdsServerBuilder in the API. See https://github.com/grpc/grpc-java/issues/7552. — committed to grpc/grpc-java by ejona86 4 years ago
- api: Expose ForwardingServerBuilder for XdsServerBuilder This reduces ABI issues caused by returning the more precise XdsServerBuilder in the API. See https://github.com/grpc/grpc-java/issues/7552. — committed to dfawley/grpc-java by ejona86 4 years ago
- core: Rewrite builder class signatures to avoid internal class This provides us a path forward with #7211 (hiding AbstractManagedChannelImplBuilder and AbstractServerImplBuilder) while providing user... — committed to ejona86/grpc-java by ejona86 3 years ago
- core: Rewrite builder class signatures to avoid internal class This provides us a path forward with #7211 (hiding AbstractManagedChannelImplBuilder and AbstractServerImplBuilder) while providing us... — committed to grpc/grpc-java by ejona86 3 years ago
Yes, it could. But that is of dubious value. New invocations of javac will still use the AbstractManagedChannelImplBuilder-returning methods.
No. It isn’t a new class. It’s actually pretty old. We can do that with ForwardingServerBuilder, but it is probably better to just remove/hide the class in the short-term.
@njhill, in your example
ForwardingChannelBuilder<T extends AbstractManagedChannelImplBuilder<T>>is redefining all the methods. So it is necessary. That signature is very bad though, as it increases the amount of code that will reference the internal class.I was suggesting having
ForwardingChannelBuilder<T extends ForwardingChannelBuilder<T>>, so that new compilations would stop referencing the internal class. For the old methods to show up with that approach,AbstractManagedChannelImplBuilderwould need to redefine all the methods.For the short-term, we’re going to copy ForwardingChannelBuilder (and server) to the old AbstractManagedChannelImplBuider name to restore the previous API. We’ll release a 1.33.1.
That will then give us time to figure out what we want to do. I am expecting we’ll need to break at least one ABI, and make use of the ExperimentalAPI allowance to do so, but we need to figure out which and how we want users to react.
On top of that, other Server and Channel Builders are affected, see v.1.33.0 release notes:
io.grpc.netty.NettyServerBuilderis no longer a subclass of the internal classio.grpc.internal.AbstractServerImplBuilderio.grpc.netty.NettyChannelBuilderis no longer a subclass of the internal classio.grpc.internal.AbstractManagedChannelImplBuilderio.grpc.okhttp.OkhttpChannelBuilderis no longer a subclass of the internal classio.grpc.internal.AbstractManagedChannelImplBuilderThe class io.grpc.inprocess.InProcessChannelBuilderis no longer a subclass of the internal classio.grpc.internal.AbstractManagedChannelImplBuilderio.grpc.cronet.CronetChannelBuilderis no longer a subclass of the internal classio.grpc.internal.AbstractManagedChannelImplBuilderEDIT: all -> other
@sergiitk, it seems 25 methods from ForwardingChannelBuilder aren’t shadowed by NettyChannelBuilder. That’s a lot. That means we’ll want to continue using ForwardingChannelBuilder.
It looks like we can change the generics of ForwardingChannelBuilder from
<T extends ForwardingChannelBuilder<T>>to<T extends ManagedChannelBuilder<T>>to avoid ForwardingChannelBuilder being defined as the return type of the overridden methods. That avoids us repeating this problem in the future. Although it will be an ABI breakage for existing users of the class.Unfortunately, we can’t change to the weaker generics in ForwardingChannelBuilder and have it extend AbstractManagedChannelImplBuilder. That makes it unclear how to proceed.
@ejona86 right, that’s what I was alluding to above. I think it should not even be necessary for such a placeholder
AbstractManagedChannelImplBuilderto explicitly impl any methods… only the constructors calling super.