Proposal: Move forwarders into io.grpc.util
- Dominant language
- Java
- Stars
- 12.1k
- Forks
- 4k
- Avg merge
- 2d 17h
- Merged PRs (30d)
- 37
Description
We have a couple classes in the top io.grpc. package that are "Forwarding" classes. These classes are generally useful on their own, and provide an interception mechanism for changing or monitoring the behavior of the forwardee. While users could implement forwarders themselves, it would be better if we did it. There are a couple reasons:
* Forwarding is error prone. As the classes we have evolve, we continually add methods that should have been abstract but cannot be due to backwards compatibility. `ManagedChannel` is one such class. Because they are not abstract, they are easy to forget to override, as has happened in the past. We are in a position to update the forwarding class as we modify the original class. Additionally, we can take on the burden of testing that each method is forwarded more easily than users.
* Forwarders act as good alternatives to mocks. Often in tests we use mocks for ease of use. This has proven to make our tests brittle and hard to understand. It would be better to make dummy classes that are suitable for test use. Forwarders act as a good base class for tests to extend. It allows an inplace migration from mocks to not mocks. Forwarders can delegate to a mock as the test is being updated. Traditionally we haven't moved test helper classes to a shared spot, but this is due to inconvenience rather than it being bad idea.
In #3105 the issue was raised that NoopClientCall and BaseClientCall were prone to abuse. Their original intent was a stand in for mocks, as many callsites needed a nonnull instance. However, because their default behavior was "do nothing", they were more easily abused. I believe forwarding classes neatly solve both problems. A forwarder can wrap a `null` instance to be used at call sites that expect a non null instance. If any methods are called, they will fail with NPE preventing users from abusing the behavior. They can define the methods they only need for their tests instead.
The reason I raise this is that there may be a lot of Forwarders if we adopt this pattern. I think they are useful in both test code and in regular code, so the logical place to put them would be in a place accessible to both. `io.grpc.util` seems appropriate, because they are supplemental to the core library.
As for the existing Forwarding classes, they will become shim classes, and their implementation will move to io.grpc.util.
As for which new Forwarders would live in the new location, the following have been on my wish list (with the first one being recently fulfilled by @dapengzhang0 )
* ForwardingManagedChannelBuilder
* ForwardingManagedChannel (people mock this class today!)
* ForwardingServerBuilder
* ForwardingLoadBalancer
Each of these would be useful to either users and/or ourselves.
@ejona86 @zhangkun83 @zpencer thoughts?
Contributor guide
Assessment
This issue has not been assessed yet.