-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
maxInboundMessageSize is not applied when app code has newer gRPC version than used in library #8313
Comments
In the example, I am modeling a real situation: the library compiled with the old version is used as a binary dependency (and cannot be recompiled in nearest future). Fortunately, I've found a workaround (the library allows you to customize |
@marx-freedom Thanks for documenting this case. That's an interesting one. As I understand, this might be not #7552 specifically, but rather it's a consequence of one of many PRs that make up the larger refactoring #7211. This refactoring broke the ABI, and #7552 rolled back some parts of the refactoring, and rolled forward other. Looks like the issue is caused in #7359 by pushing down It's possible this is another case of ABI breakage. cc @ejona86 |
This is caused by #7834.
The problem is when compiling NettyChannelBuilder the generic T doesn't extend AbstractManagedChannelImplBuilder so javac doesn't create the additional variation of the method that returns AbstractManagedChannelImplBuilder. However AbstractManagedChannelImplBuilder itself does have the method, and it just forwards, which is then thrown away by the default implementation in ManagedChannelBuilder. This isn't a problem for the (most?) other methods because they are actually implemented by ManagedChannelImplBuilder, but in this case we moved the responsibility of which class handles the method which exposes a gap. A solution is not immediately clear. I'm toying with ideas like having AbstractManagedChannelImplBuilder.maxInboundMessageSize() throw; at least then this won't be a silent issue. We should check to find any other methods that may be similarly impacted. |
BTW, @marx-freedom, good job tracking the issue down to the clear reproduction steps. I'm sure it was very confusing. |
In dbd903c we started generatating AbstractManagedChannelImplBuilder methods in a way that they are present for ABI but on recompilation only public ManagedChannelBuilder-returning methods would be used. However, this sorta missed maxInboundMessageSize which is now being handled by the concrete classes (e.g, NettyChannelBuilder) instead of ManagedChannelImplBuilder. Users on the old ABI will end up getting ManagedChannelImplBuilder.maxInboundMessageSize() which does nothing. So, let's just implement the method and have it jump back up to the concrete class. Hacky, but easy and predictable and will go away once we remove the rest of the AbstractManagedChannelImplBuilder compatibility hacks. Fixes grpc#8313
As part of refactoring described in issue grpc#7211, the implementation of this method, and its corresponding field was pulled down from internal AbstractManagedChannelImplBuilder to concrete classes that actually enforce this setting. That's the right place for method's implementation, so it wasn't ported to ManagedChannelImplBuilder too. Then AbstractManagedChannelImplBuilder was brought to fix ABI backward compatibility, and temporarily turned into a ForwardingChannelBuilder, see PR grpc#7564. Eventually it will be deleted, after a period with "bridge" solution added in grpc#7834. However, this bringing AbstractManagedChannelImplBuilder back fix unintentionally made this method's ABI backward incompatible: pre-refactoring builds expect maxInboundMessageSize() to be a method of AbstractManagedChannelImplBuilder, and not concrete classes. This problem was caught in grpc#8313. Since the end goal is to keep only this method in concrete classes the need it, to fix its ABI issue, we temporary reintroduce it to the original layer it was removed from, AbstractManagedChannelImplBuilder. This class is also temporary, and its only intention is a ABI compatibility. Once we move forward with dropping ABI compatibility (with grpc#7834), this fix is also no longer necessary, and will go away with AbstractManagedChannelImplBuilder.
As part of refactoring described in issue grpc#7211, the implementation of this method, and its corresponding field was pulled down from internal AbstractManagedChannelImplBuilder to concrete classes that actually enforce this setting. That's the right place for method's implementation, so it wasn't ported to ManagedChannelImplBuilder too. Then AbstractManagedChannelImplBuilder was brought to fix ABI backward compatibility, and temporarily turned into a ForwardingChannelBuilder, see PR grpc#7564. Eventually it will be deleted, after a period with "bridge" solution added in grpc#7834. However, this bringing AbstractManagedChannelImplBuilder back fix unintentionally made this method's ABI backward incompatible: pre-refactoring builds expect maxInboundMessageSize() to be a method of AbstractManagedChannelImplBuilder, and not concrete classes. This problem was caught in grpc#8313. Since the end goal is to keep only this method in concrete classes the need it, to fix its ABI issue, we temporary reintroduce it to the original layer it was removed from, AbstractManagedChannelImplBuilder. This class is also temporary, and its only intention is a ABI compatibility. Once we move forward with dropping ABI compatibility (with grpc#7834), this fix is also no longer necessary, and will go away with AbstractManagedChannelImplBuilder.
In refactoring described in grpc#7211, the implementation of #maxInboundMessageSize(int) (and its corresponding field) were pulled down from internal AbstractManagedChannelImplBuilder to concrete classes that actually enforce this setting. For the same reason, it wasn't ported to ManagedChannelImplBuilder (the #delegate()). Then AbstractManagedChannelImplBuilder was brought back to fix ABI backward compatibility, and temporarily turned into a ForwardingChannelBuilder, ref PR grpc#7564. Eventually it will be deleted, after a period with "bridge" ABI solution introduced in grpc#7834. However, restoring AbstractManagedChannelImplBuilder unintentionally made ABI of pre-refactoring builds expect it to be a method of AbstractManagedChannelImplBuilder, and not concrete classes, ref grpc#8313. The end goal is to keep #maxInboundMessageSize(int) only in concrete classes that enforce it. To fix method's ABI, we temporary reintroduce it to the original layer it was removed from: AbstractManagedChannelImplBuilder. This class' only intention is to provide short-term ABI compatibility. Once we move forward with dropping the ABI, both fixes are no longer necessary, and both will perish with removing AbstractManagedChannelImplBuilder.
In refactoring described in grpc#7211, the implementation of #maxInboundMessageSize(int) (and its corresponding field) were pulled down from internal AbstractManagedChannelImplBuilder to concrete classes that actually enforce this setting. For the same reason, it wasn't ported to ManagedChannelImplBuilder (the #delegate()). Then AbstractManagedChannelImplBuilder was brought back to fix ABI backward compatibility, and temporarily turned into a ForwardingChannelBuilder, ref PR grpc#7564. Eventually it will be deleted, after a period with "bridge" ABI solution introduced in grpc#7834. However, restoring AbstractManagedChannelImplBuilder unintentionally made ABI of pre-refactoring builds expect it to be a method of AbstractManagedChannelImplBuilder, and not concrete classes, ref grpc#8313. The end goal is to keep #maxInboundMessageSize(int) only in concrete classes that enforce it. To fix method's ABI, we temporary reintroduce it to the original layer it was removed from: AbstractManagedChannelImplBuilder. This class' only intention is to provide short-term ABI compatibility. Once we move forward with dropping the ABI, both fixes are no longer necessary, and both will perish with removing AbstractManagedChannelImplBuilder.
…8607) In refactoring described in #7211, the implementation of #maxInboundMessageSize(int) (and its corresponding field) were pulled down from internal AbstractManagedChannelImplBuilder to concrete classes that actually enforce this setting. For the same reason, it wasn't ported to ManagedChannelImplBuilder (the #delegate()). Then AbstractManagedChannelImplBuilder was brought back to fix ABI backward compatibility, and temporarily turned into a ForwardingChannelBuilder, ref PR #7564. Eventually it will be deleted, after a period with "bridge" ABI solution introduced in #7834. However, restoring AbstractManagedChannelImplBuilder unintentionally made ABI of pre-refactoring builds expect it to be a method of AbstractManagedChannelImplBuilder, and not concrete classes, ref #8313. The end goal is to keep #maxInboundMessageSize(int) only in concrete classes that enforce it. To fix method's ABI, we temporary reintroduce it to the original layer it was removed from: AbstractManagedChannelImplBuilder. This class' only intention is to provide short-term ABI compatibility. Once we move forward with dropping the ABI, both fixes are no longer necessary, and both will perish with removing AbstractManagedChannelImplBuilder.
Hey @marx-freedom! Good news, I tracked down exact root causes, and fixed the issue. After the fix is released, and you update your application code, |
What version of gRPC-Java are you using?
Library compiled with gRPC 1.26.0 (cannot be recompiled on demand)
Application must use a newer version (>= 1.34), gRPC 1.39.0
What is your environment?
Linux, OpenJDK 15
What did you expect to see?
Custom maxInboundMessageSize value (!= 4 MiB) is applied and works correctly
What did you see instead?
Status{code=CLIENT_RESOURCE_EXHAUSTED, issues=[gRPC error: (RESOURCE_EXHAUSTED) gRPC message exceeds maximum size 4194304: 14995791 (S_ERROR)]}
Steps to reproduce the bug
Client
with gRPC 1.39.0 in the classpath.The program will print
4194304
but value100
expected.It seems this behavior was caused by fixing issue #7552
The text was updated successfully, but these errors were encountered: